791325 - There's too much asyncness exposed by the debugger frontend

Status: RESOLVED WORKSFORME

(DevTools :: Debugger, defect, P3)

Description

[Victor Porof [:vporof][:vp]]

I think part of the mess the frontend exposes now is caused by too much granularity exposed via events. An almost trivial (delicious) amount of re-engineering of this situation may have significant benefits.

For instance, there's Debugger:Loaded and Debugger:Unloaded, both fired up only to signal DOM-related state (UI initialization readyness vs. UI invalidatednessness). These are of absolutely no benefit to any listener, since the state the frontend or client is in no way dependable, and easily mistaken for being reliable startup/shutdown events.

There's also Debugger:Connecting, which is useless because it signals "I will start connecting now", information of no value to anyone (since there's no benefit/reason in stopping a connection from happening while the debugger starts up and while UI is standing by), but also easily mistaken for being a reliable "i can start doing stuff with the debugger now" event.

We're also exposing multiple paths triggering (or asking for) for the same outcome. Consider Debugger:Close and the unload event (both listened by the parent DebuggerUI). This adds confusion. We could simply trigger a DebuggerClose event from the controller on unload, and avoid all this awkwardness.

The effects of this granularity are first of all seen in tests (there are bundles of files that adopt one paradigm, and different ones follow a different path). Of course, not all tests are the same, but we're in a state now in which some tests wait for framesadded for absolutely no reason, and other tests rely on the completely wrong events (Debugger:Loaded and Debugger:Unloaded).

By removing most of these events and carefully choosing which ones to expose, most racing conditions would be forever avoided in clients depending on the debugger frontend state (of course, there's still the issue of balance *inside* the frontend, but at least it'll "feel" solid from an outside perspective).

My suggestion is: have only two reliable events exposing startup/shutdown, mimicking the InspectorUI. These would be:
* "debugger-opened" - fired after all the UI is initialized and in standby, and after a connection succeeds, and after the initial resume
* "debugger-closed" - fired after the UI is invalidated and removed from the DOM, and disconnecting finished

Something tells me that this is everything the-outside-world needs to know about the debugger state regarding startup/shutdown. Of course, there's the current AfterScriptsAdded and AfterNewScript which are useful, so it's best to keep them.

A closer analysis of the LoadSource and friends is in order after bug 755661 (and related) settle down. It's best to clearly separate what's needed for tests and what's exposed.

Comment 1

[Girish Sharma [:Optimizer]]

will the future "debugger-opened" event be fired after everything is done, like the script list is retrieved, the first script is shown, etc . ?

Comment 2

[Victor Porof [:vporof][:vp]]

(In reply to Girish Sharma [:Optimizer] from comment #1)
> will the future "debugger-opened" event be fired after everything is done,
> like the script list is retrieved, the first script is shown, etc . ?

No, because pages with no scripts may exist, and I believe there's no way to clearly define these boundaries ("we don't have scripts" vs. "i got you all the scripts") because of unsolicited notifications.

Maybe a different "debugger-ready" fired after the first ScriptShown event (and implicitly ulterior to AfterScriptsAdded) would be useful, but I'm not sure about that.

Comment 3

[Girish Sharma [:Optimizer]]

I think that kind of notification is important, otherwise other tools [read Web Console and Timeline] would not be able to know when the script list is loaded and thus would able to load a certain file if it exits in the script list. On similar ground, they would not be able to know when the text of a selected file has been retrieved fully so that they can move the caret to desired location

Comment 4

[Victor Porof [:vporof][:vp]]

(In reply to Girish Sharma [:Optimizer] from comment #3)

Not true. We have ScriptShown.

Comment 5

[Girish Sharma [:Optimizer]]

(In reply to Victor Porof [:vp] from comment #4)
> (In reply to Girish Sharma [:Optimizer] from comment #3)
> 
> Not true. We have ScriptShown.

I know right now it is possible, I was talking about the scenarios where you want to get rid of all these and have only two events, corresponding to load and unload .

Comment 6

[Victor Porof [:vporof][:vp]]

(In reply to Girish Sharma [:Optimizer] from comment #5)
> (In reply to Victor Porof [:vp] from comment #4)
> > (In reply to Girish Sharma [:Optimizer] from comment #3)
> > 
> > Not true. We have ScriptShown.
> 
> I know right now it is possible, I was talking about the scenarios where you
> want to get rid of all these and have only two events, corresponding to load
> and unload .

No, I didn't suggest that.
I was talking about reliable initialization/destruction, removing redundancy/uselessness where possible.

Other events like AfterScriptsAdded and AfterNewScript are useful and we should keep them. In addition, stuff like ScriptShown (and friends) is also necessary, but may require an investigation on what they rely/imply on after some protocol changes take place.

Comment 7

[Victor Porof [:vporof][:vp]]

Attachment: re-enable tab navigation

Enabled tab navigation handling for ThreadState, StackFrames and SourceScripts as discussed. At first glance, this seems to do the trick, however, intermittently, the following behavior occurs:

(there are two sources avaialble, js.php?type=1 and js.php?type=2, let's call them A and B for brevity)

1. Page is refreshed, tab navigated event is fired.
2. SourceScripts tries to getSources. Only source A is retrieved as part of the response
3. Server got B as a newSource, but the frontend listener SS__onNewSource isn't fired in time
4. First breakpoint hits in B
5. The frontend tries to update the editor with source B, but the text is never retrieved

Comment 8

[Victor Porof [:vporof][:vp]]

Attachment: logs for scenario in comment #7

Comment 9

[Victor Porof [:vporof][:vp]]

Um.. wrong bug :)

Comment 10

[Girish Sharma [:Optimizer]]

(In reply to Victor Porof [:vp] from comment #9)
> Um.. wrong bug :)

I think there was too much asyncness in your opening of tab and picking a bug to edit ;)

Comment 11

[Victor Porof [:vporof][:vp]]

This isn't high priority anymore. We're fine as we are, but it may be a good idea to revisit this bug at some point, maybe when we switch to promises.

Comment 12

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Closing because we use a set hashtag promise based APIs in the frontend now.