Closed
Bug 1255565
Opened 9 years ago
Closed 9 years ago
Unnecessary extra window hanging around when running reftest
Categories
(Testing :: Reftest, defect)
Testing
Reftest
Tracking
(firefox48 fixed)
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: ahal, Assigned: ahal)
References
Details
(Keywords: regression)
Attachments
(1 file)
This is caused by bug 1245092.
Basically that bug made reftest a bootstrapped addon that gets installed at runtime by marionette to work around addons signing. Because reftest is no longer being initialized via a command line handler, it meant there was this extra window hanging around that wasn't there before. Originally the patch in bug 1245092 closed this window, but this caused intermittent memory leaks with e10s enabled.
Because this extraneous window doesn't seem to have any impact on the tests, and because there is a rush to enforce addon signing starting with 46 beta.. this was spun off into a follow-up bug.
As for the problem, my theory is that the leak is caused by marionette hanging on to a reference of that extraneous window after it was being closed. I don't know for sure though.
Comment 1•9 years ago
|
||
I would really like to see this bug fixed. It's making reftest debugging harder than it needs to be.
For some reason there seems to be a lot of painting happening in the new window, so whenever I debug paint problems (i.e. the kind of problems usually exposed by reftests), I need to manually remove or ignore any debugging output or breakpoint stops triggered by that window.
Comment 3•9 years ago
|
||
Note also that this window is booby-trapped, as discussed in (duplicate) bug 1264104. Clicks in certain places (e.g. the URL bar or the search bar) will trigger network activity, which aborts the testrun.
Comment 4•9 years ago
|
||
[Marking this as a regression from bug 1245092, to reflect reality.]
Assignee | ||
Comment 5•9 years ago
|
||
I spent far too long trying to fix this back in Q1, but eventually ran out of time and gave up (JS and gecko are not my strong suits). Here is some additional context.
Prior to bug 1245092, the reftest extension had a commandline handler which ran before the window was opened. This made sure the window was sized correctly (800x1000), had no toolbars, etc. Now, the reftest extension can't even be installed until well afterwards, the commandline handler no longer exists.
Originally my patch simply opened a new window with the proper size and settings, and then closed the old one that was still hanging around from when Firefox was first started. This worked almost perfectly, except for an intermittent windows e10s debug only memory leak, frequent enough that it would cause a backout. Here is where I was closing the first window:
https://dxr.mozilla.org/mozilla-central/source/layout/tools/reftest/bootstrap.js#52
I had basically done wm.getMostRecentWindow('navigator:browser').close(). I'll can do a new try run tomorrow. With luck, maybe the leak was known e10s issue that has since been fixed. Needinfo'ing myself to remember.
Flags: needinfo?(ahalberstadt)
Assignee | ||
Comment 6•9 years ago
|
||
Huh, so this may have automagically fixed itself:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=453b370e479aaa3a0b0aeaa83132eeb0e11408d8
To prove I'm not insane, here's the try run from before that leaked all over the place:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bdb168296e90
Anyway, I'm happy to chalk this up to the good progress being made on e10s and not worry about it. Patch coming shortly.
Flags: needinfo?(ahalberstadt)
Assignee | ||
Comment 7•9 years ago
|
||
This extra window was initially left open because closing it was causing memory leaks
on debug e10s crashtests. There was pressure to get the regressing patch landed due
to addon signing, so it got landed with this extra window hanging around (as it didn't
impact test results).
But it is a UX wart for several reasons. Upon testing it again recently, the leaks all
seem to have vanished. I'm not sure why, possibly it was a bug fixed in e10s.
Review commit: https://reviewboard.mozilla.org/r/46407/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46407/
Attachment #8741347 -
Flags: review?(dholbert)
Comment 8•9 years ago
|
||
Might have been bug 1238707.
Comment 9•9 years ago
|
||
Comment on attachment 8741347 [details]
MozReview Request: Bug 1255565 - Close extraneous browser window when running reftests, r?dholbert
https://reviewboard.mozilla.org/r/46407/#review42987
I don't really know this code, but I'll r+ this based on the fact that it was previously reviewed in the other bug, and because your explanation makes sense.
Thanks!
Attachment #8741347 -
Flags: review?(dholbert) → review+
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → ahalberstadt
Status: NEW → ASSIGNED
Comment 10•9 years ago
|
||
Comment 11•9 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•