Don't cache the docshell in WebNavigation in browser-child.js

RESOLVED FIXED in Firefox 40

Status

()

Firefox
General
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mccr8, Assigned: billm)

Tracking

unspecified
Firefox 40
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10sm6+, firefox40 fixed)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

2 years ago
When running toolkit/components/addoncompat/ we end up holding onto a docshell for too long.  Bill and I looked at this, and it seems like what is happening is that the _webNavigation field of WebNavigation keeps the docshell alive, even past when we clear the webNavigation field off of the global.  Bill wrote a patch to fix this that just makes us QI it every time we access it, avoiding the leak.
(Reporter)

Updated

2 years ago
tracking-e10s: --- → ?
(Reporter)

Comment 1

2 years ago
Created attachment 8570736 [details] [diff] [review]
Don't cache the docshell in WebNavigation in browser-child.js.

Also don't cache the session history, because we only use it in init.

(Bill wrote this patch.)
(Reporter)

Updated

2 years ago
Attachment #8570736 - Flags: review?(felipc)
(Reporter)

Comment 2

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ea0a1eaf31a
Attachment #8570736 - Flags: review?(felipc) → review+
(Reporter)

Comment 3

2 years ago
Try run was green.  It looks like this fixes up almost all of the remaining bc and dt DOCSHELL DOMWINDOW leaks, based on Bill's push to Holly.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a761ac6f8bcc
(Reporter)

Comment 4

2 years ago
This seems to be making opt builds time out in e10s bc3, in various thumbnails tests.  Very weird.

I think the timeouts are due to this exception being thrown:

09:18:10 WARNING - TEST-UNEXPECTED-FAIL | unknown test url | uncaught exception - ReferenceError: executeSoon is not defined at chrome://mochitests/content/browser/toolkit/components/thumbnails/test/head.js:228
(Reporter)

Comment 5

2 years ago
backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/d0e70d2cf479

I guess I'll look into these failures on Monday.

Updated

2 years ago
tracking-e10s: ? → m6+
Whiteboard: [MemShrink] → [MemShrink:P2]
(Assignee)

Comment 6

2 years ago
Created attachment 8585216 [details] [diff] [review]
patch v2

The original patch had a bug. There's a pre-existing comment explaining that we need to keep the sessionHistory alive. This patch regressed that. Anyway, this new patch fixes the issue and still avoids leaks.
Attachment #8570736 - Attachment is obsolete: true
Attachment #8585216 - Flags: review?(felipc)
(Assignee)

Updated

2 years ago
Depends on: 1148961
(Assignee)

Updated

2 years ago
Depends on: 1148962
(Reporter)

Updated

2 years ago
Attachment #8585216 - Attachment is patch: true
Attachment #8585216 - Flags: review?(felipc) → review+
(Assignee)

Comment 7

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/00e022f7e45e
https://hg.mozilla.org/mozilla-central/rev/00e022f7e45e
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Depends on: 1150200

Updated

2 years ago
Depends on: 1150224
No longer depends on: 1150224
You need to log in before you can comment on or make changes to this bug.