859230 - Crash when creating windowless docshell

Status: RESOLVED FIXED

(Core :: General, defect, critical)

Description

[Mark Hammond [:markh]]

Bug 846906 allows us to create windowless docshells.  Attempting to use this new feature crashes Fx (even though the new tests work, and the STR below is based directly on those tests!?).

STR:
* Open a new scratchpad and set the environment to "browser".
* Paste the following code:

  let webNavigation = Services.appShell.createWindowlessBrowser();
  let hiddenDoc = webNavigation.document;
  let iframe = hiddenDoc.createElement("iframe");
  hiddenDoc.documentElement.appendChild(iframe);

* Run it

My initial debugging shows there seems to be a reference counting bug - the crash is trying to dereference a pointer to a WebBrowserChrome2Stub which has been released.  I'll dig a little more tomorrow unless some magic fairy beats me to it :)

Comment 1

[Eddy Bruel [:ejpbruel]]

Since I wrote the windowless docshell code, I'm assigning this bug to me.

Unfortunately, my laptop screen died yesterday, and I need to take it to the shop before I can start working on this. To make matters worse, building Firefox on my replacement machine takes more than an hour. If anyone wants to pick up this bug before I get to it, feel free to reassign it.

For what it's worth: the problem seems to be very simple: the web browser instance we're returning needs a container window, but since we don't have that here, we create a stub object that implements nsIWebBrowserChrome2 and assign it to be the container window.

Unfortunately, it seems that web browsers only maintain a weak reference to their container window, so after this function returns, there is nothing keeping it alive. I'm mildly confused that the test didn't catch this, since it's testing for exactly the steps you described here. Anyone got a theory on that? (see test_bug864906.xul in dochshell/test/chrome)

Two obvious solutions suggest themselves:

1. Do what bz originally suggested and instead of returning an instance of nsIWebNavigation
   return a new object that keeps both the web browser and its container window alive.
2. *Or* make the web browser's container window a strong reference (bz: is this safe at all?)

Comment 2

[Eddy Bruel [:ejpbruel]]

Since I wrote the windowless docshell code, I'm assigning this bug to me.

Unfortunately, my laptop screen died yesterday, and I need to take it to the shop before I can start working on this. To make matters worse, building Firefox on my replacement machine takes more than an hour. If anyone wants to pick up this bug before I get to it, feel free to reassign it.

For what it's worth: the problem seems to be very simple: the web browser instance we're returning needs a container window, but since we don't have that here, we create a stub object that implements nsIWebBrowserChrome2 and assign it to be the container window.

Unfortunately, it seems that web browsers only maintain a weak reference to their container window, so after this function returns, there is nothing keeping it alive. I'm mildly confused that the test didn't catch this, since it's testing for exactly the steps you described here. Anyone got a theory on that? (see test_bug864906.xul in dochshell/test/chrome)

Two obvious solutions suggest themselves:

1. Do what bz originally suggested and instead of returning an instance of nsIWebNavigation
   return a new object that keeps both the web browser and its container window alive.
2. *Or* make the web browser's container window a strong reference (bz: is this safe at all?)

Comment 3

[Eddy Bruel [:ejpbruel]]

Boris, can you confirm everything in the above comment?

Comment 4

[Boris Zbarsky [:bzbarsky, bz on IRC]]

>1. Do what bz originally suggested and instead of returning an instance of nsIWebNavigation
>   return a new object that keeps both the web browser and its container window alive.

I still suggest that.

Note that this object could implement nsIWebNavigation and just forward it to the webbrowser.  And same thing for interface requestor.  And then it should all Just Work.

> (bz: is this safe at all?)

No, it's totally not.

Note that you could also make your stub implement nsISupportsWeakReference (and I suggest you do, I think, so you don't have to do careful cleanup), but that would mean it would randomly disappear based on gc from the perspective of the docshelltreeowner if you don't hold on to it and keep it alive.

Comment 5

[Mark Hammond [:markh]]

Attachment: Return a stub nsIWebNavigation

This patch seems to do most of what bz suggested.  It introduces a new "WindowlessBrowserStub" object which is returned by CreateWindowlessBrowser(), and holds a strong reference to both the browser and the container.  The existing stub now also derives from nsSupportsWeakReference (is that all which is necessary to have it support strong references?)

FWIW, I'm trying to use this in bug 859230, and while it seems to work, I now see messages such as '[JavaScript Error: "can't access dead object"' and "Too many nested content frames have the same url (recursion?) so giving up" - I'm not sure if they are related to the changes here or reflect a different underlying problem.  But either way, I thought it worth putting this up and getting feedback.

Comment 6

[Mark Hammond [:markh]]

(In reply to Mark Hammond (:markh) from comment #5)
> (is that all which is necessary to have it support
> strong references?)

*sigh* - I obviously meant *weak* references...

Comment 7

[Eddy Bruel [:ejpbruel]]

(In reply to Mark Hammond (:markh) from comment #5)
> Created attachment 735012 [details] [diff] [review]
> Return a stub nsIWebNavigation
> 
> This patch seems to do most of what bz suggested.  It introduces a new
> "WindowlessBrowserStub" object which is returned by
> CreateWindowlessBrowser(), and holds a strong reference to both the browser
> and the container.  The existing stub now also derives from
> nsSupportsWeakReference (is that all which is necessary to have it support
> strong references?)
> 
> FWIW, I'm trying to use this in bug 859230, and while it seems to work, I
> now see messages such as '[JavaScript Error: "can't access dead object"' and
> "Too many nested content frames have the same url (recursion?) so giving up"
> - I'm not sure if they are related to the changes here or reflect a
> different underlying problem.  But either way, I thought it worth putting
> this up and getting feedback.

As far as I can tell, this is essentially the patch I would have written, so we might as well wait for feedback from bz.

Thank your for tacking this off my back, Mark. I apologize for not putting something up myself sooner, but like I said, I'm struggling with abysmal build times while I wait for my laptop to get repaired.

Comment 8

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Comment on attachment 735012 [details] [diff] [review]
Return a stub nsIWebNavigation

>+                              public nsSupportsWeakReference {

Need to QI to it too.

Looks good otherwise.  At some point we'll want CC on all this stuff...

Comment 9

[Mark Hammond [:markh]]

Attachment: Added QI support for nsISupportsWeakReference

This is the same patch as before, but with WebBrowserChrome2Stub supporting a QI for nsISupportsWeakReference.  I was going to look into adding CC support, but smaug suggested in IRC it wasn't important now as WebBrowserChrome2Stub only keeps references to non-CC objects, and ditto for WindowlessBrowserStub as nsIWebBrowser also isn't CC-able.  Let me know if that's incorrect and you'd prefer CC support before this lands.

Comment 10

[Boris Zbarsky [:bzbarsky, bz on IRC]]

smaug's right; adding useful CC here is a big undertaking.  That's why I said "at some point", not "add it, please".  I'm sorry I wasn't clearer about that.

Comment 11

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Comment on attachment 735623 [details] [diff] [review]
Added QI support for nsISupportsWeakReference

r=me

Comment 12

[Mark Hammond [:markh]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a3b06580a0da

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a3b06580a0da