1021571 - browser_inspector_pseudoclass_lock.js | A promise chain failed to handle a rejection - unknownError

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Patrick Brosset <:pbro>]

See the logs: https://tbpl.mozilla.org/php/getParsedLog.php?id=40626296&tree=Try&full=1#error14

Required to fix bug 1018184

Comment 1

[Patrick Brosset <:pbro>]

Attachment: bug1021571-unhandled-rejected-promise-browser_inspector_pseudoclass_lock.js.patch

This patch should generally help reducing the number of exceptions we usually see when closing the toolbox (during normal use or during tests).
At least it gets rid of the unhandled rejected promise in the test, which is what we're after in this bug.

Comment 2

[Brian Grinstead [:bgrins]]

Comment on attachment 8436979 [details] [diff] [review]
bug1021571-unhandled-rejected-promise-browser_inspector_pseudoclass_lock.js.patch

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

::: toolkit/devtools/server/actors/inspector.js
@@ +935,5 @@
>    },
>  
>    destroy: function() {
>      this._hoveredNode = null;
> +    // At destroy time, no need to send mutations to the front

Looking at this, I'm thinking it may be better to have a this._destroyed=true or this.sendMutations = false and handling this in the onMutations function instead of adding a new parameter to the clearPseudoClassLocks method.

It seems like this is something that could come up with other types of mutations as well, and the extra parameter is a bit confusing since it's not part of the 'request' object.

Could do something like:

initialize: function() {
  this.sendMutations = true;
},

queueMutation: function() {
  if (!this.actorID || !this.sendMutations) {
    // We've been destroyed, don't bother queueing this mutation.
    return;
  }
},

destroy: function() {
  this.sendMutations = false;
},

What do you think about this?  Hopefully this will catch any other similar occurrences in the future.

@@ +1728,5 @@
>      if (node) {
>        DOMUtils.clearPseudoClassLocks(node.rawNode);
>        this._activePseudoClassLocks.delete(node);
> +      if (!localOnly) {
> +        this._queuePseudoClassMutation(node);

I wonder if there is a way to handle this for all mutations (this will inevitably come up

Comment 3

[Patrick Brosset <:pbro>]

(In reply to Brian Grinstead [:bgrins] from comment #2)
> Comment on attachment 8436979 [details] [diff] [review]
> bug1021571-unhandled-rejected-promise-browser_inspector_pseudoclass_lock.js.
> patch
> 
> Review of attachment 8436979 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/devtools/server/actors/inspector.js
> @@ +935,5 @@
> >    },
> >  
> >    destroy: function() {
> >      this._hoveredNode = null;
> > +    // At destroy time, no need to send mutations to the front
> 
> Looking at this, I'm thinking it may be better to have a
> this._destroyed=true or this.sendMutations = false and handling this in the
> onMutations function instead of adding a new parameter to the
> clearPseudoClassLocks method.
> 
> It seems like this is something that could come up with other types of
> mutations as well, and the extra parameter is a bit confusing since it's not
> part of the 'request' object.
> 
> Could do something like:
> 
> initialize: function() {
>   this.sendMutations = true;
> },
> 
> queueMutation: function() {
>   if (!this.actorID || !this.sendMutations) {
>     // We've been destroyed, don't bother queueing this mutation.
>     return;
>   }
> },
> 
> destroy: function() {
>   this.sendMutations = false;
> },
> 
> What do you think about this?  Hopefully this will catch any other similar
> occurrences in the future.
Yeah that looks better.
The problem we're still having is that, in many cases, mutations are being queued before the actor gets destroyed, therefore triggering an event to the front, which then requests them via 'getMutations', however in the meantime, the actor has been destroyed. And that's why we're seeing 'Error: Connection closed' when closing the browser with the toolbox open.
This is for another bug anyway, and I can definitely apply the feedback your proposed for this fix, I'm just saying that this will unfortunately not catch all situations.

Comment 4

[Patrick Brosset <:pbro>]

Attachment: bug1021571-unhandled-rejected-promise-browser_inspector_pseudoclass_lock.js v2.patch

v2 - applied feedback
Try for v1 was green: https://tbpl.mozilla.org/?tree=Try&rev=56220e79a791
New try for v2: https://tbpl.mozilla.org/?tree=Try&rev=ea892c0cc32c

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/7c389b46d920

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7c389b46d920