Closed Bug 1255565 Opened 5 years ago Closed 5 years ago
Unnecessary extra window hanging around when running reftest
58 bytes, text/x-review-board-request
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.
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.
Duplicate of this bug: 1264104
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.
[Marking this as a regression from bug 1245092, to reflect reality.]
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.
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.
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)
Might have been bug 1238707.
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: nobody → ahalberstadt
Status: NEW → ASSIGNED
You need to log in before you can comment on or make changes to this bug.