1239822 - Explicitly destroy windowless browser created to host background pages

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P1)

Description

[Kris Maglione [:kmag]]

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.

Comment 1

[Kris Maglione [:kmag]]

Attachment: 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.

Comment 2

[Kris Maglione [:kmag]]

Attachment: Part 2 - [webext] Explicitly destroy windowless browsers on unload

Comment 3

[Boris Zbarsky [:bzbarsky]]

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.

Comment 4

[Kris Maglione [:kmag]]

(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.

Comment 5

[Kris Maglione [:kmag]]

Attachment: Part 2a - [webext] Explicitly destroy windowless browsers on unload

Comment 6

[Kris Maglione [:kmag]]

Attachment: Part 2b - Destroy windowless browsers created for add-on SDK page workers

Comment 7

[Kris Maglione [:kmag]]

Attachment: Part 2c - Destroy windowless browsers created by browser parsable CSS tests

Comment 8

[:Gijs (he/him)]

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?

Comment 9

[Kris Maglione [:kmag]]

(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 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 11

[Kris Maglione [:kmag]]

(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.

Comment 12

[Kris Maglione [:kmag]]

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

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/917c8d47b1c6
https://hg.mozilla.org/mozilla-central/rev/3e121f599090
https://hg.mozilla.org/mozilla-central/rev/bd1e06031d56

Comment 14

[Kris Maglione [:kmag]]

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

Comment 15

[Kris Maglione [:kmag]]

Thanks, everyone.

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5fccc3de040f