Closed Bug 966970 Opened 9 years ago Closed 9 years ago

Intermittent browser_webconsole_bug_632275_getters_document_width.js | Test timed out

Categories

(DevTools :: Console, defect)

Product:
DevTools
Component:
Console
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox28 unaffected, firefox29 fixed, firefox30 fixed, firefox-esr24 unaffected)

Status:
RESOLVED FIXED
Milestone:
Firefox 30
Tracking Flags:
Tracking Status
firefox28 --- unaffected
firefox29 --- fixed
firefox30 --- fixed
firefox-esr24 --- unaffected

People

(Reporter: ttaubert, Assigned: msucan)

References

Details

(Keywords: intermittent-failure, Whiteboard: [qa-])

Attachments

(1 file)

bug966970-1.diff
1.76 KB, patch
pbro
: review+
Sylvestre
: approval-mozilla-aurora+
Details | Diff | Splinter Review 
https://tbpl.mozilla.org/php/getParsedLog.php?id=34001285&tree=Mozilla-Central

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser_webconsole_bug_632275_getters_document_width.js | Test timed out
Return code: 1
Attached patch bug966970-1.diff — — Splinter Review 
If you look into the TBPL logs, you will see the test runs fine and it crashes in exceptions when it calls finishTest(), then it times out.

Exceptions happen in toolbox.js and in the inspector actor. Both are related to bug 952277.

In toolbox destroy() the check for this._inspector assumes that all of the related objects are also available. You can see that in initInspector() _selection, _highlighter and _walker become available at later times - after _inspector is set.

I added some checks in toolbox destroy() which, hopefully, will allow the test to correctly finish.

I did not change the inspector actor because it is normal to return an error when we request a NodeFront for an unknown object actor ID.

Did some code reading and initInspector() also looks prone to failures. This method can be called from different places, multiple times, yet it assumes the presence of _inspector is enough for resolving the promise - which is not, as it is made clear by the function itself.
Assignee: nobody → mihai.sucan
Status: NEW → ASSIGNED
Attachment #8370700 - Flags: review?(pbrosset)
Depends on: 952277
Comment on attachment 8370700 [details] [diff] [review]
bug966970-1.diff

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

Looks good. Indeed the initInspector and destroyInspector functions were too optimistic!
I'm happy to R+ that change, but just wanted to propose an alternative, up to you to decide whether it's better or not for this patch:
in initInspector, instead of assigning the inspector front to this._inspector, the walker to this._walker and the selection to this._selection where it's done now, only do this just before resolving the promise. That means only set them on the class instance when the highlighter has been retrieved.
This way we'd be sure that these objects always go hand in hand and it would then become possible to safely do if(this._inspector) to assert that all other objects are here.
Attachment #8370700 - Flags: review?(pbrosset) → review+
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #23)
> Comment on attachment 8370700 [details] [diff] [review]
> bug966970-1.diff
> 
> Review of attachment 8370700 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good. Indeed the initInspector and destroyInspector functions were too
> optimistic!
> I'm happy to R+ that change, but just wanted to propose an alternative, up
> to you to decide whether it's better or not for this patch:
> in initInspector, instead of assigning the inspector front to
> this._inspector, the walker to this._walker and the selection to
> this._selection where it's done now, only do this just before resolving the
> promise. That means only set them on the class instance when the highlighter
> has been retrieved.
> This way we'd be sure that these objects always go hand in hand and it would
> then become possible to safely do if(this._inspector) to assert that all
> other objects are here.

I tried this, and at least one test started to fail on my system. I also tried other changes and it looks like the tests depend on the current code. As such, I'd like to keep this intermittent test failure fix minimal. Thank you for the review.

We'll see if this patch is sufficient for fixing the intermittent failures.
Landed: https://hg.mozilla.org/integration/fx-team/rev/598eff91dfdc
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/598eff91dfdc
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 30
This is blocking bug 966970 from being uplifted to Aurora. Can we land this there as well?
status-firefox29: unaffected → affected
Flags: needinfo?(mihai.sucan)
Comment on attachment 8370700 [details] [diff] [review]
bug966970-1.diff

[Approval Request Comment]
Bug caused by (feature/regressing bug #): This was a fix for an intermittent mochitest test failure that happens when the devtools toolbox gets destroyed at the end of a particular test.
Mihai changed the destroy code in this bug to make sure it doesn't fail when various things aren't initialized beforehand.
This bug is a dependency for bug 962478 which we would like to uplift to Aurora.
See the approval request comment in that bug for more information, but essentially, it's about the devtools' inspector tool polluting the content page with extra event listeners after it's closed.

User impact if declined: If declined, bug 962478 cannot apply cleanly on the mozilla-aurora branch.
bug 966970 alone has no user impacts though, cause it's about an error that only occurs during tests.

Testing completed (on m-c, etc.): try build tests done. Fixed on m-c.

Risk to taking this patch (and alternatives if risky): This patch makes the devtools' toolbox destroy process less prone to failures, so there should be no risk in taking it. Furthermore, the destroy process was only failing during tests.

String or IDL/UUID changes made by this patch: None
Attachment #8370700 - Flags: approval-mozilla-aurora?
Attachment #8370700 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #36)
> This is blocking bug 966970 from being uplifted to Aurora. Can we land this
> there as well?

Yes, I expect this patch can land in aurora. And I see you already did.
Whiteboard: [qa-]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.