Accessing properties of a non-fully initialized window crashes [@ nsDOMClassInfo::PreCreate]

RESOLVED FIXED

Status

()

Core
DOM
--
critical
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: Wladimir Palant, Unassigned)

Tracking

(4 keywords)

1.8 Branch
crash, fixed1.8.0.7, fixed1.8.1, regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(3 attachments)

(Reporter)

Description

11 years ago
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

11 years ago
Created attachment 208651 [details]
Testcase

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

Updated

11 years ago
Blocks: 348990
(Reporter)

Comment 3

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

11 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]
(In reply to comment #5)
> Got this on a FF trunk.

With the testcase?

Comment 7

11 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

11 years ago
*** Bug 341784 has been marked as a duplicate of this bug. ***

Comment 9

11 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]
Created attachment 235107 [details] [diff] [review]
suggested patch of comment 0

This seems to fix the crash I see in bug 348990.
Attachment #235107 - Flags: review?(jst)
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
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

11 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
(In reply to comment #13)
> Martijn, you didn't mean to assign this bug to me? 

Ok, sorry about that.
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 on attachment 235107 [details] [diff] [review]
suggested patch of comment 0

r-
Attachment #235107 - Flags: review?(jst) → review-
Well, bug 348990 doesn't have a minimal testcase, but it has a reproducable way of getting this crash.

Comment 18

11 years ago
As a sidenote to comment #7 I've not got this bug since I uninstalled LiveHTTPHeaders.
Created attachment 236533 [details]
crunched nsHeaderInfo.js file from livehttpheaders

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

11 years ago
FWIW, only four unique users generated those 16 crashes (in the FF2x topcrash list).
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.
Patch for bug 348990 will fix this.  This is a regression from bug 296639.
Blocks: 296639
No longer blocks: 348990
Depends on: 348990
Keywords: regression
Fixed by checkin for bug 348990.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Keywords: fixed1.8.0.7, fixed1.8.1
Resolution: --- → FIXED
Flags: in-testsuite?
Crash Signature: [@ nsDOMClassInfo::PreCreate]
You need to log in before you can comment on or make changes to this bug.