Closed Bug 1263378 Opened 8 years ago Closed 7 years ago

Intermittent browser_bug461710.js | leaked 2 window(s) until shutdown [url = chrome://browser/content/browser.xul][url = about:blank]

Categories

(Toolkit :: Places, defect, P1)

Product:
Toolkit
Component:
Places
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox-esr52 --- wontfix
firefox54 --- wontfix
firefox55 --- fixed
firefox56 --- fixed

People

(Reporter: RyanVM, Assigned: mak)

References

Details

(Keywords: intermittent-failure, memory-leak, Whiteboard: [stockwell fixed:logic][fxsearch])

Attachments

(1 file)

Bug 1263378 - Intermittent leaks from browser_bug461710.js.
59 bytes, text/x-review-board-request
standard8
: review+
Details 
https://treeherder.mozilla.org/logviewer.html#?job_id=25442237&repo=mozilla-inbound

16:44:11  WARNING -  TEST-UNEXPECTED-FAIL | toolkit/components/places/tests/browser/browser_bug461710.js | leaked 2 window(s) until shutdown [url = chrome://browser/content/browser.xul]
16:44:11  WARNING -  TEST-UNEXPECTED-FAIL | toolkit/components/places/tests/browser/browser_bug461710.js | leaked 2 window(s) until shutdown [url = about:blank]
16:44:11     INFO -  TEST-INFO | toolkit/components/places/tests/browser/browser_bug461710.js | windows(s) leaked: [pid = 3632] [serial = 82], [pid = 3632] [serial = 83], [pid = 3632] [serial = 96], [pid = 3632] [serial = 95]
Bulk assigning P3 to all open intermittent bugs without a priority set in Firefox components per bug 1298978.
Priority: -- → P3
I hope this is similar to bug 1332783.
Assignee: nobody → gbrown
(In reply to Geoff Brown [:gbrown] from comment #3)
> I hope this is similar to bug 1332783.

Actually, it seems different and I'm not making progress...will leave this for someone else.
Assignee: gbrown → nobody
This has been consistently failing 20+ times/week for many weeks, the last 2 weeks 30+ times.  As this is a leak we only detect that on debug, and this looks to be a mix of e10s/non-e10s.  We typically run this test every 5th push, so the failure rate should be much higher if we did run this every push.

::mak, I see you are the triage owner for the toolkit:places component, can you help find someone to look into this sometime in the next 2 weeks and come to a resolution (fix, reduced frequency, disabled)?
Flags: needinfo?(mak77)
Whiteboard: [stockwell needswork]
This is likely due to the usage of a private browsing window, similarly to other leaks in the past (comment 3), it's likely we somehow create a cycle there, but it may not be trivial to find that. Especially finding the underlying code in PB that may create this cycle could be very expensive.

We could try some things on Try to keep references to windows as short as possible and see if that helps, for example now each test object keeps a reference to a window.
It may help or not, I don't have further ideas and it sounds hard to reproduce this.

I could eventually try to do some test pushes on Try and see in the next days.
Priority: P3 → P1
Whiteboard: [stockwell needswork] → [stockwell needswork][fxsearch]
Doing some tests, but no promises, this is hard to reproduce.
One thought I had is that this test runs after browser_bug399606.js, that is another intermittent... So I'll try to cleanup both and see what happens.

We could also just disable this test on linux as an alternative, since it's where it happens more often.
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Flags: needinfo?(mak77)
thanks ::mak!
Comment on attachment 8880177 [details]
Bug 1263378 - Intermittent leaks from browser_bug461710.js.

https://reviewboard.mozilla.org/r/151540/#review156784

Looks reasonable, but I'm worried about the browserLoaded symantics as mentioned below.

::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm:265
(Diff revision 1)
>     *
>     * @return {Promise}
>     * @resolves When a load event is triggered for the browser.
>     */
>    browserLoaded(browser, includeSubFrames=false, wantLoad=null) {
> +    // Passing a url as second argument is a common mistake we shoudl prevent.

typo: should

::: toolkit/components/places/tests/browser/browser_bug399606.js:44
(Diff revision 1)
> -      let uri = URIs.shift();
> -      loadCount = 0;
> -      gBrowser.addEventListener("DOMContentLoaded", handleLoad, true);
> -      gBrowser.loadURI(uri);
> +    gBrowser.loadURI(uri);
> -    } else {
> -      confirm_results();
> +    await BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser, false, uri);
> +    await BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser, false, uri);

browserLoaded just waits for the browser to load, so I don't think this is actually loading three times, I think it is just adding listeners three times (and somehow manages to resolve, which is strange).
Attachment #8880177 - Flags: review?(standard8)
(In reply to Mark Banner (:standard8) from comment #27)
> browserLoaded just waits for the browser to load, so I don't think this is
> actually loading three times, I think it is just adding listeners three
> times (and somehow manages to resolve, which is strange).

hm, but each time the previous load completed, since we await.
From what I see browserLoaded waits for a load event from content (forwarded through "browser-test-utils:loadEvent"), so I don't see why it shouldn't work.
I start a load, wait for the load event, when I get it I register for another load event, and so on.
I don't think this code may register 3 events for the same load, but maybe I'm missing something :(

From my local testing it works as expected, I see the tab reloading thrice, and then being closed.

Do you have an alternative in mind?
Flags: needinfo?(standard8)
(In reply to Marco Bonardo [::mak] from comment #28)
> (In reply to Mark Banner (:standard8) from comment #27)
> > browserLoaded just waits for the browser to load, so I don't think this is
> > actually loading three times, I think it is just adding listeners three
> > times (and somehow manages to resolve, which is strange).
> 
> hm, but each time the previous load completed, since we await.
> From what I see browserLoaded waits for a load event from content (forwarded
> through "browser-test-utils:loadEvent"), so I don't see why it shouldn't
> work.
> I start a load, wait for the load event, when I get it I register for
> another load event, and so on.
> I don't think this code may register 3 events for the same load, but maybe
> I'm missing something :(
> 
> From my local testing it works as expected, I see the tab reloading thrice,
> and then being closed.

Ok, so I totally missed the fact that the html files are reloading themselves. Makes sense now...
Flags: needinfo?(standard8)
Comment on attachment 8880177 [details]
Bug 1263378 - Intermittent leaks from browser_bug461710.js.

https://reviewboard.mozilla.org/r/151540/#review156818

Looks good now! r=Standard8 with the nits addressed.

::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm:267
(Diff revision 1)
>     * @resolves When a load event is triggered for the browser.
>     */
>    browserLoaded(browser, includeSubFrames=false, wantLoad=null) {
> +    // Passing a url as second argument is a common mistake we shoudl prevent.
> +    if (includeSubFrames && typeof includeSubFrames != "boolean")
> +      throw("The second argument to browserLoaded should be a boolean.");

nit: please include braces as per the general style in the rest of the file.
Attachment #8880177 - Flags: review+
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 32e6e30110e1 -d 6304c0ecb59d: rebasing 403505:32e6e30110e1 "Bug 1263378 - Intermittent leaks from browser_bug461710.js. r=standard8" (tip)
merging testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm
merging uriloader/exthandler/tests/mochitest/browser_web_protocol_handlers.js
warning: conflicts while merging uriloader/exthandler/tests/mochitest/browser_web_protocol_handlers.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/903dc6eaad3f
Intermittent leaks from browser_bug461710.js. r=standard8
let's hope this sticks, potentially it may fix more than one intermittent since 5 tests were misusing browserLoaded.
https://hg.mozilla.org/mozilla-central/rev/903dc6eaad3f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Whiteboard: [stockwell needswork][fxsearch] → [stockwell fixed][fxsearch]
Whiteboard: [stockwell fixed][fxsearch] → [stockwell fixed:logic][fxsearch]
See Also: → 1595573
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: