886848 - Promisify the debugger frontend

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Description

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

I should've done this in bug 854185, but somehow I only stopped at communications between DebuggerPanel -> debugger-controller.js. There a few more small bits that I think would benefit from using promises.

One more baby step to using promises everywhere in the frontend.

Comment 1

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

Attachment: v1, part 1 - debugger

Comment 2

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

Attachment: v1, part 2 - netmonitor

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 767263 [details] [diff] [review]
v1, part 1 - debugger

Review of attachment 767263 [details] [diff] [review]:
-----------------------------------------------------------------

I very much like this, but can't we fix the shutdown path as well?

::: browser/devtools/debugger/debugger-controller.js
@@ +79,2 @@
>        } else {
> +        return null; // Done.

I think you mean Promise.resolve(null) here.

@@ +110,2 @@
>  
> +    // Shutdown is synchronous, for now.

Not really, due to the asynchronous way breakpoints are being destroyed and the client.close() call in chrome debugging. Why don't we fix this here as well, once and for all?

::: browser/devtools/debugger/debugger-view.js
@@ +80,2 @@
>      this._destroyPanes();
>      this._destroyEditor();

_destroyEditor calls DebuggerController.Breakpoint.destroy, which calls removeBreakpoint for every one of them, which is async. Granted, we don't have much use for its callback in the tear down path, but it would be much cleaner to just add promises to that part as well.

@@ -181,5 @@
>      dumpn("Finished loading the DebuggerView editor");
>  
>      DebuggerController.Breakpoints.initialize();
>      window.dispatchEvent(document, "Debugger:EditorLoaded", this.editor);
> -    this.editor.focus();

Why was this redundant?

Comment 4

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 767265 [details] [diff] [review]
v1, part 2 - netmonitor

Review of attachment 767265 [details] [diff] [review]:
-----------------------------------------------------------------

This seems fine!

Comment 5

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

Preliminary try run: https://tbpl.mozilla.org/?tree=Try&rev=8e12bb6f6083

Comment 6

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

Comment on attachment 767263 [details] [diff] [review]
v1, part 1 - debugger

I'll combine these.

Comment 8

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

As suggested in comment #3, I started rewriting the breakpoint shutdown paths to handle asyncess, and promises seemed like a natural fit. However, fortunately or unfortunately, lots of code paths had to be modified to account for this change, which didn't seem all so natural.

So I decided we should just go all the way and use promises whenever and wherever possible. I'd really like to silence all our oranges once and for all, and it seems that a good way to achieve that is by fixing our broken "rely on callbacks and events everywhere, but don't guard against the crazy outside world" approach.

I'm renaming this bug to "Promisify the debugger frontend" since that's what I've been doing this weekend. I've tried to split this work into several small patches to make it more obvious what changed and where.

Naturally, this will cause havok in tests, but in the interest of smoothing the review process I'll add all the changes here except test fixes.

Note that this bug alone will *not* magically fix all of our oranges, but I plan to modify the tests and use the new APIs soon in bug 876277.

Comment 9

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

Attachment: Part 1: Chain promises on debugger initialization/destruction to remove some redundancy

This was the first patch for this bug, and the first step in promisifying the frontend. It makes sure we're not mixing callbacks and promises in the initialization path and lays the foundations for the next patches.

Comment 10

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

Attachment: Part 2: Convert the breakpoints in the controller to use promises

The second step was to account for the asyncness in breakpoints removal that wasn't handled until now, as pointed out in comment #3. To make everything fit more naturally, I converted the breakpoints store into an actual breakpoint promises map, which helps account for any unexpected race conditions when adding and removing breakpoints.

Comment 11

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

Attachment: Part 3: Convert the debugger frontend to use the new breakpoint APIs

Now, the debugger view was relying heavily on the old callbacks mechanism with respect to breakpoints, so I had to change most of it to handle the new APIs. I also modified it to use promises in tandem with the controller wherever it made sense. This results in a massive code cleanup and, IMHO, a more straightforward approach to adding and removing breakpoints in the view.

Comment 12

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

Attachment: Part 4: Update debugger commands to use the new breakpoint APIs

Obviously, the commands had to be updated as well to account for the API changes.

Comment 13

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

Attachment: Part 5: Convert the editor to rely more on Promises

Handling and displaying sources was a complete mess, and race conditions were everywhere. This patch switches the frontend to using a single point of entry for changing the currently displayed source url and caret line number, and uses promises to make sure things don't get out of sync.

Comment 14

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

Attachment: Part 6: Cleanup, fix other typos, rename a few variables for consistency

This patch is mostly directed at housekeeping, commenting things where I felt that they weren't completely obvious and removing some dead code. All of the stuff that has been bugging me for a while but I didn't manage to find the time to cleanup.

Comment 15

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

I filed bug 891442 for the netmonitor patch (comment #4).

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/877550e7041b

Comment 17

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

Not yet :)

Although originating from this bug, the patch that landed was morphed into its own bug: 891442. My bad for forgetting to change the bug number when landing.

Comment 18

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 772720 [details] [diff] [review]
Part 1: Chain promises on debugger initialization/destruction to remove some redundancy

Review of attachment 772720 [details] [diff] [review]:
-----------------------------------------------------------------

s/Promise/promise/ everywhere. Presumably this will apply to the rest of the patches.

::: browser/devtools/debugger/debugger-controller.js
@@ +341,5 @@
> +      } else {
> +        deferred.resolve(quit.data);
> +        Services.startup.quit(Ci.nsIAppStartup.eForceQuit);
> +      }
> +    }}, 0);

Hm, this may not work when we switch to async promises, but that's for me to look at in bug 881050.

::: browser/devtools/debugger/debugger-view.js
@@ -181,5 @@
>      dumpn("Finished loading the DebuggerView editor");
>  
>      DebuggerController.Breakpoints.initialize();
>      window.dispatchEvent(document, "Debugger:EditorLoaded", this.editor);
> -    this.editor.focus();

I've asked this before, but I don't think you answered it: why was this redundant?

Comment 19

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

(In reply to Panos Astithas [:past] from comment #18)
> 
> ::: browser/devtools/debugger/debugger-controller.js
> @@ +341,5 @@
> > +      } else {
> > +        deferred.resolve(quit.data);
> > +        Services.startup.quit(Ci.nsIAppStartup.eForceQuit);
> > +      }
> > +    }}, 0);
> 
> Hm, this may not work when we switch to async promises, but that's for me to
> look at in bug 881050.
> 

You're right that this will not work, but quitting is sync anyway so nobody's really going to care anymore whether the promise is resolved or not. The rejection part is what's mostly important in here. Maybe I should just remove the resolve line?

> ::: browser/devtools/debugger/debugger-view.js
> @@ -181,5 @@
> >      dumpn("Finished loading the DebuggerView editor");
> >  
> >      DebuggerController.Breakpoints.initialize();
> >      window.dispatchEvent(document, "Debugger:EditorLoaded", this.editor);
> > -    this.editor.focus();
> 
> I've asked this before, but I don't think you answered it: why was this
> redundant?

Previously, because the source editor was the only focusable thing in the debugger, it had to receive focus on start to allow for keyboard shortcuts to work (don't ask me how or why that was the case, I don't know). Now, we have all sorts of focusable things, most notably the sidemenuwidget which gets auto focus on startup (because it's the first focusable node in the DOM). Hence, focusing the editor is not required anymore.

Comment 20

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Victor Porof [:vp] from comment #19)
> (In reply to Panos Astithas [:past] from comment #18)
> > Hm, this may not work when we switch to async promises, but that's for me to
> > look at in bug 881050.
> > 
> 
> You're right that this will not work, but quitting is sync anyway so
> nobody's really going to care anymore whether the promise is resolved or
> not. The rejection part is what's mostly important in here. Maybe I should
> just remove the resolve line?

Yes, I guess you are right. I wouldn't remove deferred.resolve, it's not wrong and it makes the code easier to follow.

Comment 21

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 772722 [details] [diff] [review]
Part 2: Convert the breakpoints in the controller to use promises

Review of attachment 772722 [details] [diff] [review]:
-----------------------------------------------------------------

I tried real hard to find something wrong with this patch, but nope.

::: browser/devtools/debugger/debugger-controller.js
@@ +1112,4 @@
>  }
>  
>  Breakpoints.prototype = {
> +  get activeThread() DebuggerController.activeThread,

Not required, but since you are touching this, maybe it would be better to use a traditional getter as explained in bug 887895?

@@ +1193,5 @@
>  
> +    // Initialize the breakpoint, but don't update the editor, since this
> +    // callback is invoked because a breakpoint was added in the editor itself.
> +    this.addBreakpoint(location, { noEditorUpdate: true }).then(aBreakpointClient => {
> +      // If the breakpoint client has an "requestedLocation" attached, then

s/an/a/

@@ +1309,5 @@
>      this.activeThread.setBreakpoint(aLocation, (aResponse, aBreakpointClient) => {
> +      // If there was an error adding the breakpoint, reject the promise
> +      // and forget about it so that the breakpoint may be re-added later.
> +      if (aResponse.error) {
> +        deferred.reject(aResponse);

We should add some UI do display such errors in the front-end. Not in this bug of course.

@@ +1388,5 @@
> +        // If there was an error removing the breakpoint, reject the promise
> +        // and forget about it that the breakpoint may be re-removed later.
> +        if (aResponse.error) {
> +          deferred.reject(aResponse);
> +          return void this._removing.delete(identifier);

IIANM this will cause another SpiderMonkey-specific strict warning due to this function not always returning a value. If that is the case, then please, for the love of god and all that is holy, just do a simple "...delete(); return;". I hate that stupid warning, but in this case we can easily avoid it.

@@ +1413,5 @@
> +   *         rejected if there was an error.
> +   */
> +  removeAllBreakpoints: function() {
> +    /* Gets an array of all the existing breakpoints promises. */
> +    let breakpointsActive = (aPromises, aStore = []) => {

If this was a parameterless getter then I would have loved it, but functions sound better as verbs, so how about getActiveBreakpoints and getRemovedBreakpoints?

Comment 22

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

(In reply to Panos Astithas [:past] from comment #21)
> 
> If this was a parameterless getter then I would have loved it, but functions
> sound better as verbs, so how about getActiveBreakpoints and
> getRemovedBreakpoints?

Certainly. What I tried to do is make them sound better in tandem with Promise.all: "all breakpoints active, then ... all breakpoints removed, then ...".

Comment 23

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 772724 [details] [diff] [review]
Part 3: Convert the debugger frontend to use the new breakpoint APIs

Review of attachment 772724 [details] [diff] [review]:
-----------------------------------------------------------------

Clearing the review request due to the async execution issues mentioned below, but otherwise this looks good.

::: browser/devtools/debugger/debugger-controller.js
@@ +968,5 @@
>    _onSourcesAdded: function(aResponse) {
>      if (aResponse.error) {
> +      let msg = "Error getting sources: " + aResponse.message;
> +      Cu.reportError(msg);
> +      dumpn(msg);

We should copy the backend's reportError() helper function at some point.

::: browser/devtools/debugger/debugger-panes.js
@@ +394,5 @@
> +    let breakpointPromise = DebuggerController.Breakpoints._getAdded(attachment);
> +    if (breakpointPromise) {
> +      breakpointPromise.then(aBreakpointClient => {
> +        isConditionalBreakpoint = "conditionalExpression" in aBreakpointClient;
> +        conditionalExpression = aBreakpointClient.conditionalExpression;

How can this work if the breakpointPromise isn't resolved yet? Won't conditionalExpression get the actual contents after the rest of _openConditionalPopup has already executed?

@@ +651,5 @@
> +    let isConditionalBreakpoint = false;
> +    let breakpointPromise = DebuggerController.Breakpoints._getAdded(attachment);
> +    if (breakpointPromise) {
> +      breakpointPromise.then(aBreakpointClient => {
> +        isConditionalBreakpoint = "conditionalExpression" in aBreakpointClient;

Same here: isConditionalBreakpoint won't be set before this.highlightBreakpoint() is called.

@@ +864,5 @@
> +      enableOthers();
> +    } else {
> +      gThreadClient.interrupt();
> +      gThreadClient.addOneTimeListener("paused", () => {
> +        enableOthers().then(() => gThreadClient.resume());

No need to listen to the paused event. Just add this function as a callback to the interrupt() method. Might want to check if the callback's response parameter is an error, too.

Comment 24

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 772726 [details] [diff] [review]
Part 4: Update debugger commands to use the new breakpoint APIs

Review of attachment 772726 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/debugger/CmdDebugger.jsm
@@ -235,5 @@
> -      });
> -    } catch (ex) {
> -      console.error('Error removing breakpoint, already removed?', ex);
> -      // If the debugger has been closed already, don't scare the user.
> -      deferred.resolve(gcli.lookup("breakdelRemoved"));

Is this case handled properly now? I'm not sure from looking at the code. Can you test it please?

r=me with this verified.

Comment 25

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

(In reply to Panos Astithas [:past] from comment #23)
> How can this work if the breakpointPromise isn't resolved yet? Won't
> conditionalExpression get the actual contents after the rest of
> _openConditionalPopup has already executed?
> 

Indeed. If the promise is not resolved, then the conditional popup just isn't shown. 

However, it's highly unlikely that it won't be resolved when this happens. This is even better than what was previously happening, where we weren't even checking if the breakpoint was actually added.

But you're right, I need to reword/recode this a bit to prevent such cases, if we're going to honest about handling every possible scenario.

> @@ +864,5 @@
> > +      enableOthers();
> > +    } else {
> > +      gThreadClient.interrupt();
> > +      gThreadClient.addOneTimeListener("paused", () => {
> > +        enableOthers().then(() => gThreadClient.resume());
> 
> No need to listen to the paused event. Just add this function as a callback
> to the interrupt() method. Might want to check if the callback's response
> parameter is an error, too.

Good idea.

Comment 26

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

(In reply to Panos Astithas [:past] from comment #24)
> Is this case handled properly now? I'm not sure from looking at the code.
> Can you test it please?
> 
> r=me with this verified.

Yes, see the previous if (!dbg) { ... } check. Also, if the debugger closes *while* removing a breakpoint, then the removal will either succeed or rejected (depends on the timing), but both of these cases are also taken care of.

Comment 27

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 772729 [details] [diff] [review]
Part 5: Convert the editor to rely more on Promises

Review of attachment 772729 [details] [diff] [review]:
-----------------------------------------------------------------

Just one questionable change for the default highlighting mode, but this looks great otherwise!

::: browser/devtools/debugger/debugger-panes.js
@@ +623,5 @@
>      if (!sourceItem) {
>        return;
>      }
>      // The container is not empty and an actual item was selected.
> +    DebuggerView.setEditorLocation(sourceItem.value);

The comment seems out of place now. It would also be cleaner to guard the line above with the inverse check I think, instead of returning early before an one-liner. Nitpicking!

::: browser/devtools/debugger/debugger-view.js
@@ +27,5 @@
>  const SEARCH_TOKEN_FLAG = "#";
>  const SEARCH_LINE_FLAG = ":";
>  const SEARCH_VARIABLE_FLAG = "*";
> +const DEFAULT_EDITOR_CONFIG = {
> +  mode: SourceEditor.MODES.TEXT,

Why this change? IIRC we used JS as the default for the case of web pages or scripts without correct content types from the backend.

@@ +308,1 @@
>     * @param object aFlags [optional]

So you didn't rename this to aOptions, eh? OK.

Comment 28

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

(In reply to Panos Astithas [:past] from comment #27)
> Comment on attachment 772729 [details] [diff] [review]
> Part 5: Convert the editor to rely more on Promises
> 
> Review of attachment 772729 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Just one questionable change for the default highlighting mode, but this
> looks great otherwise!
> 
> ::: browser/devtools/debugger/debugger-panes.js
> @@ +623,5 @@
> >      if (!sourceItem) {
> >        return;
> >      }
> >      // The container is not empty and an actual item was selected.
> > +    DebuggerView.setEditorLocation(sourceItem.value);
> 
> The comment seems out of place now. It would also be cleaner to guard the
> line above with the inverse check I think, instead of returning early before
> an one-liner. Nitpicking!
> 

You're totally right, not nitpicking.

> ::: browser/devtools/debugger/debugger-view.js
> @@ +27,5 @@
> >  const SEARCH_TOKEN_FLAG = "#";
> >  const SEARCH_LINE_FLAG = ":";
> >  const SEARCH_VARIABLE_FLAG = "*";
> > +const DEFAULT_EDITOR_CONFIG = {
> > +  mode: SourceEditor.MODES.TEXT,
> 
> Why this change? IIRC we used JS as the default for the case of web pages or
> scripts without correct content types from the backend.
> 
> @@ +308,1 @@
> >     * @param object aFlags [optional]
> 
> So you didn't rename this to aOptions, eh? OK.

I should've. The reasons for all the previous renamings were that aFlags imply boolean flags IMHO. Since these aren't boolean flags, they should also be renamed. Thanks for catching this.

Comment 29

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

(In reply to Panos Astithas [:past] from comment #27)

Missed a comment!

> Why this change? IIRC we used JS as the default for the case of web pages or
> scripts without correct content types from the backend.
> 

These flags specify the initial state of the source editor, not perpetual defaults. We're changing the mode anyway every time when setting a source. And we *always* pick a text mode first for displaying the "Loading..." string. So switching for an initial JS mode to a TEXT mode "just because" didn't seem right.

Comment 30

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 772732 [details] [diff] [review]
Part 6: Cleanup, fix other typos, rename a few variables for consistency

Review of attachment 772732 [details] [diff] [review]:
-----------------------------------------------------------------

Neat.

::: browser/devtools/debugger/debugger-panes.js
@@ +1838,5 @@
>  
>    /**
>     * Gets the number of source results in this store.
>     */
> +  get matchCount() this._store.length

Regular getter while you're here?

@@ +1873,2 @@
>     */
> +  get matchCount() this._store.length,

Same here.

@@ +1961,4 @@
>      resultsContainer.className = "dbg-results-container";
>      resultsContainer.setAttribute("hidden", "true");
>  
> +    // Create lines search results entries and add them to this container.

"Create lines *for* search result entries"

@@ +2027,5 @@
>  
>    /**
> +   * Gets the number of word results in this store.
> +   */
> +  get matchCount() this._matchCount,

And here.

Comment 31

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Victor Porof [:vp] from comment #29)
> These flags specify the initial state of the source editor, not perpetual
> defaults. We're changing the mode anyway every time when setting a source.
> And we *always* pick a text mode first for displaying the "Loading..."
> string. So switching for an initial JS mode to a TEXT mode "just because"
> didn't seem right.

Ah, good point.

Thanks again for splitting the patch in byte-sized chunks! This would have taken a lot more time to review otherwise!

Comment 32

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

Attachment: Part 1, rebased

Comment 33

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

Attachment: Part 2, rebased

Comment 34

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

Attachment: Part 3, addressed comments

Comment 35

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

Attachment: Part 4, rebased

Comment 36

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

Attachment: Part 5, rebased

Comment 37

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

Attachment: Part 6, rebased

Comment 38

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 777150 [details] [diff] [review]
Part 3, addressed comments

Review of attachment 777150 [details] [diff] [review]:
-----------------------------------------------------------------

Looking good.

Comment 39

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

Attachment: Part 1, rebased

Rebasing all the things.

Comment 40

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

Attachment: Part 2, rebased

Comment 41

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

Attachment: Part 2.1

Remove _skipEditorBreakpointCallbacks since it's redundant after using promises when handling breakpoints. This makes the code easier to follow and removes the cognitive overhead required to deal with setting the flag to true/false everywhere.

Comment 42

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

Attachment: Part 3, rebased

Comment 43

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

Attachment: Part 4, rebased

Comment 44

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

Attachment: Part 5, rebased

Comment 45

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

Attachment: Part 6, rebased

Comment 46

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 789104 [details] [diff] [review]
Part 2.1

Review of attachment 789104 [details] [diff] [review]:
-----------------------------------------------------------------

OK, but you missed another instance of _skipEditorBreakpointCallbacks in part 2.

Comment 47

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

(In reply to Panos Astithas [:past] from comment #46)
> Comment on attachment 789104 [details] [diff] [review]

I'm not sure what you mean. Part 2.1 is responsible with removing all of them.

Comment 48

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

Gah, apparently splinter is once again having trouble reading patches. I wish I understood why.

Comment 49

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

https://hg.mozilla.org/integration/fx-team/rev/114177e01bda
https://hg.mozilla.org/integration/fx-team/rev/5ce1bf575b28
https://hg.mozilla.org/integration/fx-team/rev/d1ac0e8c3300
https://hg.mozilla.org/integration/fx-team/rev/a926ec1e9582
https://hg.mozilla.org/integration/fx-team/rev/10b1453a6d9d
https://hg.mozilla.org/integration/fx-team/rev/628a2db4b46b
https://hg.mozilla.org/integration/fx-team/rev/ba5df19635da

Comment 50

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/114177e01bda
https://hg.mozilla.org/mozilla-central/rev/5ce1bf575b28
https://hg.mozilla.org/mozilla-central/rev/d1ac0e8c3300
https://hg.mozilla.org/mozilla-central/rev/a926ec1e9582
https://hg.mozilla.org/mozilla-central/rev/10b1453a6d9d
https://hg.mozilla.org/mozilla-central/rev/628a2db4b46b
https://hg.mozilla.org/mozilla-central/rev/ba5df19635da