Closed
Bug 859230
Opened 12 years ago
Closed 12 years ago
Crash when creating windowless docshell
Categories
(Core :: General, defect)
Tracking
()
RESOLVED
FIXED
mozilla23
People
(Reporter: markh, Assigned: markh)
References
Details
(Keywords: crash)
Attachments
(1 file, 1 obsolete file)
4.07 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
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•12 years ago
|
||
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?)
Assignee: nobody → ejpbruel
Comment 3•12 years ago
|
||
Boris, can you confirm everything in the above comment?
Flags: needinfo?(bzbarsky)
![]() |
||
Comment 4•12 years ago
|
||
>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.
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 5•12 years ago
|
||
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.
Attachment #735012 -
Flags: feedback?(bzbarsky)
Assignee | ||
Comment 6•12 years ago
|
||
(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•12 years ago
|
||
(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•12 years ago
|
||
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...
Attachment #735012 -
Flags: feedback?(bzbarsky) → feedback+
Assignee | ||
Comment 9•12 years ago
|
||
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.
Assignee: ejpbruel → mhammond
Attachment #735012 -
Attachment is obsolete: true
Attachment #735623 -
Flags: review?(bzbarsky)
![]() |
||
Comment 10•12 years ago
|
||
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•12 years ago
|
||
Comment on attachment 735623 [details] [diff] [review]
Added QI support for nsISupportsWeakReference
r=me
Attachment #735623 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 12•12 years ago
|
||
Comment 13•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in
before you can comment on or make changes to this bug.
Description
•