Last Comment Bug 371124 - Crash [@ LookupUCProperty] with unminimised testcase, using stirdom scriptfuzzer and xbl bindings
: Crash [@ LookupUCProperty] with unminimised testcase, using stirdom scriptfuz...
Status: RESOLVED FIXED
[sg:critical?] null-deref on 1.8 bran...
: crash, testcase, verified1.8.0.12, verified1.8.1.4
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- critical (vote)
: ---
Assigned To: Mats Palmgren (vacation)
: Hixie (not reading bugmail)
Mentors:
Depends on:
Blocks: 326633
  Show dependency treegraph
 
Reported: 2007-02-21 05:28 PST by Martijn Wargers [:mwargers] (not working for Mozilla)
Modified: 2009-04-24 10:54 PDT (History)
8 users (show)
dveditz: blocking1.8.1.4+
dveditz: wanted1.8.1.x+
dveditz: blocking1.8.0.12+
dveditz: wanted1.8.0.x+
bob: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
reduced testcase (251 bytes, text/html)
2007-02-21 16:24 PST, Jesse Ruderman
no flags Details
Testcase #4 (115 bytes, text/html)
2007-02-22 13:34 PST, Mats Palmgren (vacation)
no flags Details
Patch rev. 1 (3.26 KB, patch)
2007-02-22 13:36 PST, Mats Palmgren (vacation)
no flags Details | Diff | Splinter Review
Patch rev. 1 (diff -w) (3.11 KB, patch)
2007-02-22 13:43 PST, Mats Palmgren (vacation)
jst: review+
jst: superreview+
dveditz: approval1.8.1.3+
dveditz: approval1.8.0.12+
Details | Diff | Splinter Review

Description Martijn Wargers [:mwargers] (not working for Mozilla) 2007-02-21 05:28:39 PST
Created attachment 255889 [details]
zipped testcase

To reproduce the crash:
- Extract the zipped testcase
- Open file 'parentframe.htm'

Usually the crash happens within 20 seconds or so.

Talkback ID: TB29540796Z
LookupUCProperty  [mozilla/js/src/jsapi.c, line 2642]
JS_HasUCProperty  [mozilla/js/src/jsapi.c, line 3009]
nsHTMLExternalObjSH::GetProperty  [mozilla/dom/src/base/nsdomclassinfo.cpp, line 8850]
XPC_WN_Helper_GetProperty  [mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp, line 962]
js_GetProperty  [mozilla/js/src/jsobj.c, line 3515]
js_ValueToIterator  [mozilla/js/src/jsiter.c, line 395]
js_Interpret  [mozilla/js/src/jsinterp.c, line 2764]

I haven't tried minimising it yet (this could be very tough) or getting a regression range (which also might be difficult, because I could encounter crashes unrelated to this crash).
Marking security sensitive, because it also crashes on branch (with a different stack).
Comment 1 Jesse Ruderman 2007-02-21 16:23:30 PST
Created attachment 255962 [details]
reduced testcase
Comment 2 Jesse Ruderman 2007-02-21 16:24:16 PST
Created attachment 255964 [details]
reduced testcase
Comment 5 Martijn Wargers [:mwargers] (not working for Mozilla) 2007-02-21 17:00:06 PST
(In reply to comment #3)
> That was hard to reduce.
Heh, yeah, I had a feeling that was hard to reduce, thanks for doing!
Comment 6 Martijn Wargers [:mwargers] (not working for Mozilla) 2007-02-21 18:17:54 PST
With the reduced testcase, the regression range I get is between 2006-05-18/2006-05-23 on trunk and 2006-07-06/2006-07-07 on branch, so I think this  a regression from the js1.7 landing, see:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=MOZILLA_1_8_BRANCH&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006-07-06+03&maxdate=2006-07-07+09&cvsroot=%2Fcvsroot
Comment 7 Mats Palmgren (vacation) 2007-02-22 13:34:52 PST
Created attachment 256075 [details]
Testcase #4

A similar crash occurs from nsHTMLDocumentSH::DocumentAllTagsNewResolve()
after setting "document.all.tags.__proto__ = null"
Comment 8 Mats Palmgren (vacation) 2007-02-22 13:36:11 PST
Created attachment 256077 [details] [diff] [review]
Patch rev. 1
Comment 9 Mats Palmgren (vacation) 2007-02-22 13:43:14 PST
Created attachment 256080 [details] [diff] [review]
Patch rev. 1 (diff -w)

This fixes the crash for both testcases on trunk and branches.
On the 1.8.0 branch we don't reach nsHTMLExternalObjSH::GetProperty()
with attachment 255964 [details] for some reason... the code is the same though
so the crash would likely occur if we do get there with a null proto...
Comment 10 Boris Zbarsky [:bz] 2007-02-22 13:44:45 PST
I don't really know this code well enough to do a fast review.  Ask jst?
Comment 11 Johnny Stenback (:jst, jst@mozilla.com) 2007-02-22 14:20:27 PST
Comment on attachment 256080 [details] [diff] [review]
Patch rev. 1 (diff -w)

- In nsHTMLDocumentSH::DocumentAllHelperNewResolve():

+    JSObject *proto = ::JS_GetPrototype(cx, obj);
+    if (proto) {

Rather than going past this at all in the case where we have no proto I think I'd prefer to simply bail early here (return JS_TRUE). It's not like we care to expose anything at all in any way if someone sets our document prototype's prototype (i.e. the document.all helper protos proto) to null. If we do what this patch does then an element with name="getElementById" would be reflected as document.getElementById, and that's IMO not desired here.

r+sr=jst with that change.
Comment 12 Mats Palmgren (vacation) 2007-02-22 18:58:03 PST
Fixed DocumentAllHelperNewResolve() as suggested above.
Checked in to trunk at 2007-02-22 18:18 PST.

-> FIXED
Comment 13 Daniel Veditz [:dveditz] 2007-02-27 15:21:35 PST
I get a different stack in FF2.0.0.2 and FF1.5.0.10 -- FF2 doesn't look too bad, a null-deref in LookupUCProperty. In 1.5.0.10 I crash on an uninitialized cx->fp (0xCCCCCCCC) in js_AllocStack()
Comment 14 Daniel Veditz [:dveditz] 2007-03-16 14:35:57 PDT
Comment on attachment 256080 [details] [diff] [review]
Patch rev. 1 (diff -w)

approved for 1.8/1.8.0 branches, a=dveditz for drivers
Comment 15 Mats Palmgren (vacation) 2007-04-03 09:31:56 PDT
Checked in to branches at 2007-04-03 07:47 PDT
Comment 16 Marcia Knous [:marcia - use ni] 2007-05-08 15:04:46 PDT
While trying to verify this bug, I crashed using the first zipped testcase. My Talkback Incident ID=31937866. I was using Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.8.1.4pre) Gecko/2007050804 BonEcho/2.0.0.4pre on Windows Vista. Martijn filed Bug 371125 to address this. The other testcases do not crash, so verifiying this fixed on the 1.8 branch and adding verified keyword, per discussion with gavin and martijn on IRC.
Comment 17 Marcia Knous [:marcia - use ni] 2007-05-08 16:34:44 PDT
verified on the 1.8.0 branch using Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.8.0.12pre) Gecko/20070508 Firefox/1.5.0.12pre. I did not crash with the testcases listed, including the original zipped testcase. Adding branch verified keyword.
Comment 18 Bob Clary [:bc:] 2009-04-24 10:54:57 PDT
crash test landed
http://hg.mozilla.org/mozilla-central/rev/b8a601568262

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