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

RESOLVED FIXED in Firefox 55

Status

()

defect
P1
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: RyanVM, Assigned: mak)

Tracking

(Blocks 1 bug, {intermittent-failure, memory-leak})

unspecified
mozilla56
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox54 wontfix, firefox55 fixed, firefox56 fixed)

Details

(Whiteboard: [stockwell fixed:logic][fxsearch])

Attachments

(1 attachment)

Reporter

Description

3 years ago
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]

Comment 1

3 years ago
Bulk assigning P3 to all open intermittent bugs without a priority set in Firefox components per bug 1298978.
Priority: -- → P3
Comment hidden (Intermittent Failures Robot)
I hope this is similar to bug 1332783.
Assignee: nobody → gbrown
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
(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
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
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]
Assignee

Comment 22

2 years ago
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]
Assignee

Comment 23

2 years ago
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 hidden (mozreview-request)

Comment 27

2 years ago
mozreview-review
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)
Assignee

Comment 28

2 years ago
(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 30

2 years ago
mozreview-review
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+
Comment hidden (mozreview-request)

Comment 32

2 years ago
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)
Comment hidden (mozreview-request)

Comment 34

2 years ago
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/903dc6eaad3f
Intermittent leaks from browser_bug461710.js. r=standard8
Assignee

Comment 35

2 years ago
let's hope this sticks, potentially it may fix more than one intermittent since 5 tests were misusing browserLoaded.
Assignee

Updated

2 years ago
Blocks: 1335700
Assignee

Updated

2 years ago
Blocks: 1372802
Assignee

Updated

2 years ago
Blocks: 1359227

Comment 36

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/903dc6eaad3f
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Whiteboard: [stockwell needswork][fxsearch] → [stockwell fixed][fxsearch]
Whiteboard: [stockwell fixed][fxsearch] → [stockwell fixed:logic][fxsearch]
Comment hidden (Intermittent Failures Robot)
You need to log in before you can comment on or make changes to this bug.