Closed
Bug 323641
Opened 19 years ago
Closed 18 years ago
Accessing properties of a non-fully initialized window crashes [@ nsDOMClassInfo::PreCreate]
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: ecfbugzilla, Unassigned)
References
Details
(4 keywords)
Crash Data
Attachments
(3 files)
I've only seen this in FF 1.5 but from the look of it trunk should also crash in the same place sometimes.
Steps to reproduce:
1. Install a "select" event handler on the tabbrowser element.
2. When the event fires - retrieve .contentWindow.controllers.getControllerForCommand or .contentWindow.pkcs11.addmodule (no need to call the methods).
3. Open a new tab.
Typically the browser will crash trying to create a wrapper if you do that. Talkback incident ID is TB13992337Z (http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=2&type=iid&id=TB13992337Z), there are also a few similar crashes: http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=1&searchby=stacksig&match=contains&searchfor=nsDOMClassInfo%3A%3APreCreate
It looks to me that the problem is here:
if (piwin->IsOuterWindow()) {
*parentObj = ((nsGlobalWindow *)piwin.get())->
GetCurrentInnerWindowInternal()->GetGlobalJSObject();
}
GetCurrentInnerWindowInternal() returns null. The code should check for it, like this:
if (piwin->IsOuterWindow()) {
nsGlobalWindow *innerWin = ((nsGlobalWindow *)piwin.get())->
GetCurrentInnerWindowInternal();
if (innerWin) {
*parentObj = innerWin->GetGlobalJSObject();
}
}
The crash is triggered by an extension but I'll try to make a simple testcase.
Reporter | ||
Comment 1•19 years ago
|
||
This testcase crashes Firefox 1.5 for me. In order for it to work it has to be opened it through the chrome: protocol, simply requesting privileges doesn't work. After opening pressing Ctrl-T (open a new tab) should crash the browser.
Comment 2•18 years ago
|
||
Is this still an issue? This seems to work for me, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1b1) Gecko/20060814 BonEcho/2.0b1
Reporter | ||
Comment 3•18 years ago
|
||
No, it doesn't crash for me either, so at least this situation isn't a problem any more. But the code I cited above is still in nsDOMClassInfo::PreCreate and missing a null-check so I would rather not close this bug - unless somebody explains why GetCurrentInnerWindowInternal() can't return null here.
Comment 4•18 years ago
|
||
Yeah, I found a case where it crashes, it depends on an extension, see bug 348990.
Well, adding a null-check shouldn't be too hard, would it?
Comment 5•18 years ago
|
||
Got this on a FF trunk.
TB22417120X
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060822 Minefield/3.0a1 ID:2006082204 [cairo]
Comment 6•18 years ago
|
||
(In reply to comment #5)
> Got this on a FF trunk.
With the testcase?
Comment 7•18 years ago
|
||
(In reply to comment #6)
> (In reply to comment #5)
> > Got this on a FF trunk.
>
> With the testcase?
No, that's just a random crash while browsing.
Also I got TB22422755K which happened when I kept pushing down the backspace key to go back the history, probably it accessed non-initilized things in skipping pages back. Bug 341784 is a dupe.
Comment 8•18 years ago
|
||
*** Bug 341784 has been marked as a duplicate of this bug. ***
Comment 9•18 years ago
|
||
The testcase is harmless for Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060823 Minefield/3.0a1 ID:2006082304 [cairo]
Comment 10•18 years ago
|
||
This seems to fix the crash I see in bug 348990.
Attachment #235107 -
Flags: review?(jst)
Comment 11•18 years ago
|
||
And this is number 22 on the talkback crash list for FF2:
http://talkback-public.mozilla.org/reports/firefox/FF2x/index.html
And number 8 on the talkback crash list for FF1.5:
http://talkback-public.mozilla.org/reports/firefox/FF15x/index.html
So if this is the good patch and safe enough, I think it would make sense to land it also on the branches.
Assignee: general → trev
Comment 12•18 years ago
|
||
Johnny and I talked about this a week ago, and we didn't patch this then because we're not sure what the right behavior in this case is. In particular, we were worried about something being parented to the outer window when we weren't expecting it. Another option would be to call EnsureInnerWindow from this code.
Reporter | ||
Comment 13•18 years ago
|
||
Martijn, you didn't mean to assign this bug to me? I don't know the implications of this code well enough. Reassigning back to default assignee.
Assignee: trev → general
Comment 14•18 years ago
|
||
(In reply to comment #13)
> Martijn, you didn't mean to assign this bug to me?
Ok, sorry about that.
Comment 15•18 years ago
|
||
Like Blake said, this adding a null check here isn't the right fix for this problem, it might fix the crash, but it would leave us with JS objects parented at an outer window which is generally asking for security problems.
Calling EnsureInnerWindow() from this code is definitely safer, but ideally we'd get a reproducible testcase so that we can investigate this further and get a better understanding of what actually triggers this and possibly fixing this in an even better way.
Comment 16•18 years ago
|
||
Attachment #235107 -
Flags: review?(jst) → review-
Comment 17•18 years ago
|
||
Well, bug 348990 doesn't have a minimal testcase, but it has a reproducable way of getting this crash.
Comment 18•18 years ago
|
||
As a sidenote to comment #7 I've not got this bug since I uninstalled LiveHTTPHeaders.
Comment 19•18 years ago
|
||
From talkback ID: 22767488
Tryed to download FlashFxp from download.com. Reproducible: always. click on
the "download now" link:
http://www.download.com/FlashFXP/3000-2160-10037696.html?part=dl-FlashFXP&subj=dl&tag=button
Indeed, this always crashes for me when clicking on the download now link, so
this is a much more reliable way of crashing with the livehttpheaders
extension.
So basically I ripped out the whole livehttpheaders extension, by emptying the chrome.manifest file. It is still crashing after that.
Then I started minimising the nsHeaderInfo.js file, which is in the extension's components directory. The file I attached is the most minimised I could get it, while it is still crashing.
Comment 20•18 years ago
|
||
(In reply to comment #11)
> And this is number 22 on the talkback crash list for FF2:
> http://talkback-public.mozilla.org/reports/firefox/FF2x/index.html
The problem with that link is that the numbers are so small that they are easily skewed by a single tester investigating a crash. On the just-released FF2b2 I wouldn't call this a topcrash (#120)
http://talkback-public.mozilla.org/reports/firefox/FF2b2/FF2b2-topcrashers.html
Comment 21•18 years ago
|
||
(In reply to comment #20)
> The problem with that link is that the numbers are so small that they are
> easily skewed by a single tester investigating a crash. On the just-released
> FF2b2 I wouldn't call this a topcrash (#120)
> http://talkback-public.mozilla.org/reports/firefox/FF2b2/FF2b2-topcrashers.html
In that case, I guess I could be responsible for this. I crashed a few times with the Firefox2.0 branch (not really sure how many times, I sent the talkback ID, though).
Comment 22•18 years ago
|
||
FWIW, only four unique users generated those 16 crashes (in the FF2x topcrash list).
Comment 23•18 years ago
|
||
Which is not to say we shouldn't worry about it. We did change wrappers on js components (bug 345991) which did seem to make the problem worse or more reproducable (bug 348990), and several extensions--not to mention Firefox itself--use js components.
Comment 24•18 years ago
|
||
Patch for bug 348990 will fix this. This is a regression from bug 296639.
Depends on: 348990
Updated•18 years ago
|
Keywords: regression
Comment 25•18 years ago
|
||
Fixed by checkin for bug 348990.
Updated•17 years ago
|
Flags: in-testsuite?
Updated•13 years ago
|
Crash Signature: [@ nsDOMClassInfo::PreCreate]
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•