Clean up Live Development agent loading

[iwehrman]

Currently, Live Development agents resolve their load() promises before remote services they rely on have been completely enabled. For example, NetworkAgent.load() calls Inspector.Network.enable() but does not wait for this asynchronous call to complete before returning. This pull request delays completion of these load calls until the necessary asynchronous calls have successfully completed.

I'm not aware of any problems caused by the current behavior, but fewer race conditions seems better than more race conditions. Startup races like this also tend to manifest themselves during automated testing.

[redmunds]

When playing round with disabling the DOMAgent, I discovered that the CSSAgent and RemoteAgent both depend on Inspector.Page being enabled so they get the loadEventFired event, so this needs to be moved out of DOMAgent.

[redmunds]

This issue is captured in #2599, but I thought I'd mention it here :)

[jasonsanjose]

Moved Inspector.Page.enable() to LiveDevelopment onInterstitialPageLoad.

[redmunds]

In loadAgents(), agent names are synchronously added to the _loadedAgentNames array after calling agents[name].load(). They should only be added if the call to load() is resolved.

[redmunds]

I don't think we want to take this for Sprint 22. Those are all of the comments I had.

[jasonsanjose]

Looks like this will miss the Sprint 23 boat. It's probably too late to take this change. @iwehrman, can you address @redmunds' comments for sprint 24? Thanks.

[iwehrman]

I think the requested changes to loadAgents() are unnecessary and/or out of scope here.

It shouldn't matter that the addition to _loadedAgentNames is synchronous because _loadedAgentNames is only used by unload(). Calls to unload() should happen after the promises returned by load() have resolved---though, see the caveat below---but even if they don't this just means that the agent names will be added sooner rather than later. And the agents should be unloaded regardless of whether they resolve successfully or unsuccessfully because, in case the load fails, the call to unload() is used to clean up after the failure. (We could alternatively change all the agents so they always call unload() on themselves if they fail, but I'm not sure that would be any better.)

Unfortunately, there is yet another problematic race between loadAgents() and unloadAgents that this patch doesn't address: _onDisconnect, and hence unloadAgents(), can be fired before loadAgents() has finished, or even started. Note that this is a problem with or without this patch. Ideally _onDisconnect() would be changed to block until all the agents have finished loading one way or another. But it's not completely obvious how to do this, and I don't have time to make that change right now. A new bug should be filed for this race.

[redmunds]

I agree that the issue I have with the _loadedAgentNames list does not seem to cause any known problem. It just seems to imply that it contains a list of successfully loaded agents and someone may try to use that for something in the future. If it simply contains the same list as _enabledAgentNames, then it's not needed, right? If it is needed, then it should be accurate. Or maybe the name should be changed to describe what's actually in the list?

[iwehrman]

According to the text of loadAgents(), I don't believe _loadedAgentsNames contains the same list as _enabledAgentNames. Yes, it might be safe to eliminate this list entirely and unload all possible agents in unloadAgents(). But the race I mentioned above would still exist and still be problematic.

I agree that this function sucks, but I don't immediately know how/have time to fix it the right way. I think a new bug is appropriate here. The changes in this patch don't introduce any new problems with this function (as far as I can tell) because some agents are already loading asynchronously.

[redmunds]

I opened a new issue for the _loadedAgentNames cleanup: #3390

[redmunds]

@jasonsanjose It sounds like the changes in this pull requests are related to the Live Dev research you are doing. I was hoping to merge this soon, so let me know if I should hold off.

[jasonsanjose]

Thanks for the heads up. I'll let you know soon.

[jasonsanjose]

Instead of adding a callback each time (as shown here) I was wondering if we could update Inspector._send to always return a promise if no explicit callback is specified.

[iwehrman]

I suppose so. I don't have a good feeling though for whether that can be done reliably across all API calls and whether that would have any other ramifications.

[jasonsanjose]

I made the change to Inspector._send to fix #3402 (in pull #3442). I'm not concerned about the reliability since the debugger API always sends a response for every method call. I think it makes sense if I reconcile your changes into my bug fix. What do you think?

[iwehrman]

Yes, I think that makes a lot of sense. If you're bangin' away on Live Development this sprint, you might also take a look at the race I mentioned a few comments up.

[jasonsanjose]

I probably won't dig into the loadAgents/unloadAgents race condition here since it doesn't immediately impact the Chrome 27 bug as far as I know. Randy did file a new bug #3390, so I'll reference your comment there.

[jasonsanjose]

Closing this pull request to manually reconcile with #3442.