Last Comment Bug 323641 - Accessing properties of a non-fully initialized window crashes [@ nsDOMClassInfo::PreCreate]
: Accessing properties of a non-fully initialized window crashes [@ nsDOMClassI...
Status: RESOLVED FIXED
: crash, fixed1.8.0.7, fixed1.8.1, regression
Product: Core
Classification: Components
Component: DOM (show other bugs)
: 1.8 Branch
: All All
: -- critical (vote)
: ---
Assigned To: general
: Hixie (not reading bugmail)
: Andrew Overholt [:overholt]
Mentors:
: 341784 (view as bug list)
Depends on: 348990
Blocks: splitwindows
  Show dependency treegraph
 
Reported: 2006-01-16 10:35 PST by Wladimir Palant
Modified: 2007-07-15 17:17 PDT (History)
11 users (show)
bzbarsky: in‑testsuite?
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Testcase (583 bytes, text/html)
2006-01-16 11:21 PST, Wladimir Palant
no flags Details
suggested patch of comment 0 (904 bytes, patch)
2006-08-23 12:19 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
jst: review-
Details | Diff | Splinter Review
crunched nsHeaderInfo.js file from livehttpheaders (1.69 KB, application/x-javascript)
2006-09-02 06:19 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details

Description Wladimir Palant 2006-01-16 10:35:41 PST
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.
Comment 1 Wladimir Palant 2006-01-16 11:21:57 PST
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.
Comment 2 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-08-17 03:33:20 PDT
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
Comment 3 Wladimir Palant 2006-08-17 07:27:38 PDT
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 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-08-17 10:01:58 PDT
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 Ryuichi KUBUKI 2006-08-23 08:22:51 PDT
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 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-08-23 10:37:05 PDT
(In reply to comment #5)
> Got this on a FF trunk.

With the testcase?

Comment 7 Ryuichi KUBUKI 2006-08-23 10:46:42 PDT
(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 Ryuichi KUBUKI 2006-08-23 10:50:50 PDT
*** Bug 341784 has been marked as a duplicate of this bug. ***
Comment 9 Ryuichi KUBUKI 2006-08-23 10:56:07 PDT
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 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-08-23 12:19:36 PDT
Created attachment 235107 [details] [diff] [review]
suggested patch of comment 0

This seems to fix the crash I see in bug 348990.
Comment 11 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-08-23 12:23:30 PDT
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.
Comment 12 Blake Kaplan (:mrbkap) 2006-08-23 12:25:40 PDT
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.
Comment 13 Wladimir Palant 2006-08-23 12:40:37 PDT
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.
Comment 14 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-08-23 12:41:53 PDT
(In reply to comment #13)
> Martijn, you didn't mean to assign this bug to me? 

Ok, sorry about that.
Comment 15 Johnny Stenback (:jst, jst@mozilla.com) 2006-08-23 15:32:47 PDT
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 Johnny Stenback (:jst, jst@mozilla.com) 2006-08-23 15:33:18 PDT
Comment on attachment 235107 [details] [diff] [review]
suggested patch of comment 0

r-
Comment 17 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-08-23 15:39:30 PDT
Well, bug 348990 doesn't have a minimal testcase, but it has a reproducable way of getting this crash.
Comment 18 Ryuichi KUBUKI 2006-08-27 17:03:49 PDT
As a sidenote to comment #7 I've not got this bug since I uninstalled LiveHTTPHeaders.
Comment 19 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-09-02 06:19:26 PDT
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.
Comment 20 Daniel Veditz [:dveditz] 2006-09-06 17:08:55 PDT
(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 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-09-06 17:11:17 PDT
(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 Adam Guthrie 2006-09-06 17:30:41 PDT
FWIW, only four unique users generated those 16 crashes (in the FF2x topcrash list).
Comment 23 Daniel Veditz [:dveditz] 2006-09-06 17:42:19 PDT
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 Boris Zbarsky [:bz] (still a bit busy) 2006-09-06 17:43:04 PDT
Patch for bug 348990 will fix this.  This is a regression from bug 296639.
Comment 25 Boris Zbarsky [:bz] (still a bit busy) 2006-09-08 21:43:34 PDT
Fixed by checkin for bug 348990.

Note You need to log in before you can comment on or make changes to this bug.