Explicitly destroy windowless browser created to host background pages

RESOLVED FIXED in Firefox 46

Status

()

Toolkit
WebExtensions: Untriaged
P1
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: kmag, Assigned: kmag)

Tracking

unspecified
mozilla46
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 fixed)

Details

(Whiteboard: triaged)

Attachments

(4 attachments, 1 obsolete attachment)

(Assignee)

Description

2 years ago
The docshells for our windowless browsers are currently destroyed when the windowless browser stub is GCed. This tends to happen when we're creating a new window, and it isn't safe to run scripts. Since the docshell destructor needs to run script, this causes a lot of problems.

I'm not sure that relying on JS to always destroy the windowless browsers before they're released is the right solution. I'm planning to add a close() method to the windowless browser stub to explicitly destroy the nsWebBrowser, but also a runnable to stub's destructor to destroy/release the browser when it's safe, if necessary.
(Assignee)

Comment 1

2 years ago
Created attachment 8708106 [details] [diff] [review]
Part 1 - Add a close method to windowless browsers, and only destroy when safe

I went with a new interface that subclasses nsIWebNavigation for compatibility
with existing code. But I don't see any add-ons using this method, and other
in-tree uses should be updated to explicitly close the browsers anyway, so it
might not be a bad idea to make it an incompatible change instead.
Attachment #8708106 - Flags: review?(bzbarsky)
(Assignee)

Comment 2

2 years ago
Created attachment 8708107 [details] [diff] [review]
Part 2 - [webext] Explicitly destroy windowless browsers on unload
(Assignee)

Updated

2 years ago
Comment on attachment 8708106 [details] [diff] [review]
Part 1 - Add a close method to windowless browsers, and only destroy when safe

>+    if (nsContentUtils::IsSafeToRunScript()) {

Why not just unconditionally post the runnable, but using nsContentUtils::AddScriptRunner, so it runs immediately if IsSafeToRunScript, and as soon as it becomes safe otherwise?

Or put another way, why does the else branch here have an NS_ERROR?  Is it intended as emergency recovery if someone forgets to call close() and everyone should be calling close?

If that's what we want, I'd still vote for the simpler always-async code, but just with an assert on IsSafeToRunScript() here.

r=me with that.
Flags: needinfo?(kmaglione+bmo)
Attachment #8708106 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 4

2 years ago
(In reply to Boris Zbarsky [:bz] from comment #3)
> Comment on attachment 8708106 [details] [diff] [review]
> Part 1 - Add a close method to windowless browsers, and only destroy when
> safe
> 
> >+    if (nsContentUtils::IsSafeToRunScript()) {
> 
> Why not just unconditionally post the runnable, but using
> nsContentUtils::AddScriptRunner, so it runs immediately if
> IsSafeToRunScript, and as soon as it becomes safe otherwise?
> ...
> If that's what we want, I'd still vote for the simpler always-async code,
> but just with an assert on IsSafeToRunScript() here.

I was mainly concerned that it would cause intermittent window leak detection
failures in mochitests if it was always async. It looks like AddScriptRunner
shouldn't have that problem, though, so it does seem like a better idea.

> Or put another way, why does the else branch here have an NS_ERROR?  Is it
> intended as emergency recovery if someone forgets to call close() and
> everyone should be calling close?

Yes. I think everyone should be calling close, but if someone forgets, or
fails to for some other reason, it should be handled safely.

Thanks.
Flags: needinfo?(kmaglione+bmo)
(Assignee)

Comment 5

2 years ago
Created attachment 8708564 [details] [diff] [review]
Part 2a - [webext] Explicitly destroy windowless browsers on unload
Attachment #8708564 - Flags: review?(wmccloskey)
(Assignee)

Updated

2 years ago
Attachment #8708107 - Attachment is obsolete: true
(Assignee)

Comment 6

2 years ago
Created attachment 8708565 [details] [diff] [review]
Part 2b - Destroy windowless browsers created for add-on SDK page workers
Attachment #8708565 - Flags: review?(gkrizsanits)
(Assignee)

Comment 7

2 years ago
Created attachment 8708566 [details] [diff] [review]
Part 2c - Destroy windowless browsers created by browser parsable CSS tests
Attachment #8708566 - Flags: review?(gijskruitbosch+bugs)

Comment 8

2 years ago
Comment on attachment 8708566 [details] [diff] [review]
Part 2c - Destroy windowless browsers created by browser parsable CSS tests

Review of attachment 8708566 [details] [diff] [review]:
-----------------------------------------------------------------

Oh look, this code has changed since I last looked at it...

Aside: these are cases where I'm sad not everyone uses mozreview yet, because the patch does not have enough context to see what "windowless" is (and no reasonable amount of -U<N> would have fixed that). Was there a particular reason to use splinter here?
Attachment #8708566 - Flags: review?(gijskruitbosch+bugs) → review+
(Assignee)

Comment 9

2 years ago
(In reply to :Gijs Kruitbosch from comment #8)
> Aside: these are cases where I'm sad not everyone uses mozreview yet,
> because the patch does not have enough context to see what "windowless" is
> (and no reasonable amount of -U<N> would have fixed that). Was there a
> particular reason to use splinter here?

I've never managed to make mozreview work without generating an unreasonable
amount of spam when I'm not submitting all patches for review at the same
time. Hopefully that will get better at some point. I'll try to remember to
always use mozreview when I flag you in the future.

Thanks.
Comment on attachment 8708106 [details] [diff] [review]
Part 1 - Add a close method to windowless browsers, and only destroy when safe

Review of attachment 8708106 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpfe/appshell/nsAppShellService.cpp
@@ +448,5 @@
>    NS_FORWARD_NSIWEBNAVIGATION(mWebNavigation->)
>    NS_FORWARD_NSIINTERFACEREQUESTOR(mInterfaceRequestor->)
> +
> +  NS_IMETHOD
> +  Close() override

What do you think about nulling out mWebNavigation/mInterfaceRequestor here and using NS_FORWARD_SAFE_NSIINTERFACEREQUESTOR? It seems a little safer than relying on a destroyed docshell to do the right thing if someone makes a mistake.
Attachment #8708564 - Flags: review?(wmccloskey) → review+
(Assignee)

Comment 11

2 years ago
(In reply to Bill McCloskey (:billm) from comment #10)
> What do you think about nulling out mWebNavigation/mInterfaceRequestor here
> and using NS_FORWARD_SAFE_NSIINTERFACEREQUESTOR? It seems a little safer
> than relying on a destroyed docshell to do the right thing if someone makes
> a mistake.

That makes sense. I thought about nulling them out, but I didn't want to deal
with the forwarding issues. I didn't know about NS_FORWARD_SAFE.
(Assignee)

Comment 12

a year ago
https://hg.mozilla.org/integration/fx-team/rev/917c8d47b1c66e79342d3aa89009b3e0161eca8a
Bug 1239822: Part 1 - Add a close method to windowless browsers, and only destroy when safe. r=bz

https://hg.mozilla.org/integration/fx-team/rev/3e121f5990908e2a398592993debaab084d25467
Bug 1239822: Part 2a - [webext] Explicitly destroy windowless browsers on unload. r=billm

https://hg.mozilla.org/integration/fx-team/rev/bd1e06031d56c394edab771684b44e636763bc5f
Bug 1239822: Part 2c - Destroy windowless browsers created by browser parsable CSS tests. r=gijs
(Assignee)

Updated

a year ago
Keywords: leave-open

Comment 13

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/917c8d47b1c6
https://hg.mozilla.org/mozilla-central/rev/3e121f599090
https://hg.mozilla.org/mozilla-central/rev/bd1e06031d56
Attachment #8708565 - Flags: review?(gkrizsanits) → review+

Updated

a year ago
Priority: -- → P1
Whiteboard: triaged
(Assignee)

Comment 14

a year ago
https://hg.mozilla.org/integration/fx-team/rev/5fccc3de040f8768c5466581f8157a0e6492d3e3
Bug 1239822: Part 2b - Destroy windowless browsers created for add-on SDK page workers.
(Assignee)

Comment 15

a year ago
Thanks, everyone.
Keywords: leave-open

Comment 16

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5fccc3de040f
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.