Closed
Bug 1021571
Opened 11 years ago
Closed 11 years ago
browser_inspector_pseudoclass_lock.js | A promise chain failed to handle a rejection - unknownError
Categories
(DevTools :: Inspector, defect)
DevTools
Inspector
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 33
People
(Reporter: pbro, Assigned: pbro)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
bgrins
:
review+
|
Details | Diff | Splinter Review |
See the logs: https://tbpl.mozilla.org/php/getParsedLog.php?id=40626296&tree=Try&full=1#error14
Required to fix bug 1018184
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → pbrosset
Blocks: 1018184
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: x86 → All
Assignee | ||
Comment 1•11 years ago
|
||
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.
Attachment #8436979 -
Flags: review?(bgrinstead)
Comment 2•11 years ago
|
||
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
Attachment #8436979 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 3•11 years ago
|
||
(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.
Assignee | ||
Comment 4•11 years ago
|
||
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
Attachment #8436979 -
Attachment is obsolete: true
Attachment #8437529 -
Flags: review?(bgrinstead)
Updated•11 years ago
|
Attachment #8437529 -
Flags: review?(bgrinstead) → review+
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 5•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 33
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•