Last Comment Bug 393770 - Crash [@ nsCachedStyleData::GetStyleData] from [@ nsQuoteNode::Text]
: Crash [@ nsCachedStyleData::GetStyleData] from [@ nsQuoteNode::Text]
Status: VERIFIED FIXED
[sg:critical?] wfm trunk. using freed...
: crash, verified1.8.0.14, verified1.8.1.8
Product: Core
Classification: Components
Component: General (show other bugs)
: 1.8 Branch
: All All
: -- critical (vote)
: ---
Assigned To: Mats Palmgren (vacation)
:
Mentors:
Depends on: 403090
Blocks:
  Show dependency treegraph
 
Reported: 2007-08-26 09:38 PDT by Paul Nickerson
Modified: 2011-06-13 10:01 PDT (History)
10 users (show)
dveditz: blocking1.8.1.8+
dveditz: wanted1.8.1.x+
dveditz: wanted1.8.0.x+
mats: in‑testsuite?
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
stack for the crash (3.22 KB, text/plain)
2007-08-26 13:30 PDT, Mats Palmgren (vacation)
no flags Details
Tracing the quotes list (11.07 KB, text/html)
2007-08-26 13:36 PDT, Mats Palmgren (vacation)
no flags Details
Here's the stack when that frame destroy occurs (8.97 KB, text/plain)
2007-08-26 13:38 PDT, Mats Palmgren (vacation)
no flags Details
wip (branch 1.8) (1.18 KB, patch)
2007-08-26 13:51 PDT, Mats Palmgren (vacation)
no flags Details | Diff | Splinter Review
wip2 (branch 1.8) (5.43 KB, patch)
2007-08-27 14:21 PDT, Mats Palmgren (vacation)
no flags Details | Diff | Splinter Review
wip3 (branch 1.8) (17.80 KB, patch)
2007-08-28 18:32 PDT, Mats Palmgren (vacation)
no flags Details | Diff | Splinter Review
wip4 (branch 1.8) (17.90 KB, patch)
2007-08-29 11:36 PDT, Mats Palmgren (vacation)
jonas: review+
Details | Diff | Splinter Review
wip5 (branch 1.8) (19.01 KB, patch)
2007-10-02 19:02 PDT, Mats Palmgren (vacation)
bzbarsky: superreview+
dveditz: approval1.8.1.8+
dveditz: approval1.8.0.14+
Details | Diff | Splinter Review

Description Paul Nickerson 2007-08-26 09:38:32 PDT
Created attachment 278310 [details]
testcase

Firefox version:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.6) Gecko/20070812 BonEcho/2.0.0.6

Details:
eax=02717b70 ebx=023ac510 ecx=0271c260 edx=0270ba54 esi=0012fb70 edi=02717b28
eip=02717a75 esp=0012fb3c ebp=0012fb5c iopl=0         nv up ei pl nz na po cy
cs=001b  ss=0023  ds=0023  es=0023  fs=003b  gs=0000             efl=00000203
02717a75 224402a8        and     al,byte ptr [edx+eax-58h]  ds:0023:04e2356c=??


Disassembly:
gklayout!nsIFrame::GetStateBits+0xa:
0197f53a 8b4024          mov     eax,dword ptr [eax+24h]
0197f53d 8be5            mov     esp,ebp
0197f53f 5d              pop     ebp
0197f540 c3              ret
0197f541 cc              int     3
0197f542 cc              int     3
0197f543 cc              int     3
0197f544 cc              int     3

Stack trace (using release build - debug build didnt crash):
0x2717a75
xpcom_core!nsQueryInterface::operator()
firefox!Java_netscape_javascript_JSObject_initClass
firefox!nsFontList::Release
firefox!jpeg_mem_init
firefox!nsPrintOptions::GetNewPrintSettings
firefox!nsPrintOptions::GetNewPrintSettings
firefox!nsPrintOptions::GetNewPrintSettings
firefox!nsPrintOptions::GetNewPrintSettings
firefox!nsPrintOptions::GetNewPrintSettings
xpcom_core!PL_HandleEvent
xpcom_core!PL_ProcessPendingEvents
xpcom_core!PL_IsQueueNative

This crashes all over the place, and definitely looks exploitable, IMO. Also, I wasn't able to reduce the testcase any further - maybe someone else will have more luck.
Comment 1 Paul Nickerson 2007-08-26 09:38:55 PDT
[STACKHASH:AD5295AC3574BB6B0A3CB7DBA1455E8]
Comment 2 Martijn Wargers [:mwargers] (not working for Mozilla) 2007-08-26 09:46:07 PDT
Paul, could you create a testcase that crashes online, directly?
You have external references to localhost in your source.
Comment 3 Paul Nickerson 2007-08-26 10:27:06 PDT
Created attachment 278312 [details]
fixed testcase

This should work. If anyone can further reduce this testcase, they should do so since it exposes the existence and partial workings of the fuzzer scripts.
Comment 4 Martijn Wargers [:mwargers] (not working for Mozilla) 2007-08-26 10:55:16 PDT
Thanks.
This somehow was fixed on trunk between 2006-05-17 and 2006-05-17:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006-05-17+07&maxdate=2006-05-18+09&cvsroot=%2Fcvsroot
The only bug I see in that range that might somehow be involved is bug 329122.
Comment 5 Mats Palmgren (vacation) 2007-08-26 13:30:17 PDT
Created attachment 278327 [details]
stack for the crash
Comment 6 Mats Palmgren (vacation) 2007-08-26 13:36:58 PDT
Created attachment 278328 [details]
Tracing the quotes list

Frames can be destroyed while nsQuoteList::RecalcAll is iterating the list.
nsCSSFrameConstructor::NotifyDestroyingFrame removes the corresponding
quote list node for the destroyed frame...
Comment 7 Mats Palmgren (vacation) 2007-08-26 13:38:33 PDT
Created attachment 278329 [details]
Here's the stack when that frame destroy occurs
Comment 8 Mats Palmgren (vacation) 2007-08-26 13:51:04 PDT
Created attachment 278330 [details] [diff] [review]
wip (branch 1.8)

This avoids the crash.  Looking at nsCounterList::RecalcAll(), I think the
same thing could happen there also:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/base/nsCounterManager.cpp&rev=3.1.12.1&root=/cvsroot&mark=184#163
Comment 9 Boris Zbarsky [:bz] 2007-08-26 15:53:16 PDT
So on trunk this is OK because no one sees these events, right?  Can we make that change on branch too?  Just not propagate mutation events out of anonymous content?
Comment 10 Olli Pettay [:smaug] 2007-08-27 00:57:51 PDT
Mutation events shouldn't propagate from native anonymous content on branch 
either, hmm, unless the nsIContent is a nsGenericDOMDataNode. Is that the case here? Should the same thing be done for nsGenericDOMDataNode as what was done
for nsGenericElement in Bug 382681.
Problem is that on branch nsGenericDOMDataNode doesn't implement
SetNativeAnonymous()
http://lxr.mozilla.org/mozilla1.8/source/content/base/src/nsGenericDOMDataNode.cpp#742
Comment 11 Boris Zbarsky [:bz] 2007-08-27 04:52:47 PDT
Yes, the anon content is a nsTextNode here.

Can we use GetBindingParent() == this as the native anonymous test on branch?
Comment 12 Olli Pettay [:smaug] 2007-08-27 06:04:50 PDT
Unfortunately no, because bug 329122 is trunk only :(
Comment 13 Boris Zbarsky [:bz] 2007-08-27 10:32:09 PDT
Ah, right.  :(
Comment 14 Mats Palmgren (vacation) 2007-08-27 14:21:56 PDT
Created attachment 278444 [details] [diff] [review]
wip2 (branch 1.8)
Comment 15 Mats Palmgren (vacation) 2007-08-27 14:22:26 PDT
(In reply to comment #9)
> So on trunk this is OK because no one sees these events, right?

Yes, I disabled the code that prevents these events on trunk and got
the exact same crash on trunk.

> Can we make that change on branch too?

I (naively) implemented nsGenericDOMDataNode::SetNativeAnonymous()
by setting a flag on nsGenericDOMDataNode and used that to avoid calling
HandleDOMEvent() in nsGenericDOMDataNode::HandleDOMEvent().
This fixes the bug, but we'd need to find a place for the extra bit...
(the two bits we have in mParentPtrBits are already in use AFAICT)
Comment 16 Boris Zbarsky [:bz] 2007-08-28 10:18:34 PDT
Could we steal a bit from the length in nsTextFragment?
Comment 17 Jonas Sicking (:sicking) PTO Until July 5th 2007-08-28 15:27:58 PDT
Even better would be to use a bit from mNodeInfoManager
Comment 18 Mats Palmgren (vacation) 2007-08-28 18:32:17 PDT
Created attachment 278694 [details] [diff] [review]
wip3 (branch 1.8)

Like this?
Comment 19 Mats Palmgren (vacation) 2007-08-29 11:36:32 PDT
Created attachment 278813 [details] [diff] [review]
wip4 (branch 1.8)

Correction for SetNativeAnonymous().
Comment 20 Jonas Sicking (:sicking) PTO Until July 5th 2007-08-29 11:46:41 PDT
Yes, though I would rename mNodeInfoManager to mNodeInfoManagerPtrBits to avoid someone using it wrongly accidentally.
Comment 21 Jonas Sicking (:sicking) PTO Until July 5th 2007-08-31 18:35:34 PDT
Comment on attachment 278813 [details] [diff] [review]
wip4 (branch 1.8)

r=me if you rename mNodeInfoManager to mNodeInfoManagerPtrBits and add a define for the 0x1 flag used both in Get/SetNativeAnonymous and in GetNodeInfoManager
Comment 22 Daniel Veditz [:dveditz] 2007-09-25 11:29:32 PDT
since vendors sometimes backport the patches in the bugs rather than the actual checkins (which is very naughty of them), please request branch approval on a patch that incorporates Jonas's comment 21
Comment 23 Daniel Veditz [:dveditz] 2007-10-01 15:31:36 PDT
Mats: is this going to make the 1.8.1.8 code freeze (Oct 3)?
Comment 24 Mats Palmgren (vacation) 2007-10-02 19:02:33 PDT
Created attachment 283290 [details] [diff] [review]
wip5 (branch 1.8)

Addressing Jonas' review comment.
Comment 25 Daniel Veditz [:dveditz] 2007-10-02 21:58:29 PDT
Comment on attachment 283290 [details] [diff] [review]
wip5 (branch 1.8)

approved for 1.8.1.8 and 1.8.0.14, a=dveditz
Comment 26 Mats Palmgren (vacation) 2007-10-02 22:45:18 PDT
Comment on attachment 283290 [details] [diff] [review]
wip5 (branch 1.8)

guess I need an sr here before checkin...
Comment 27 Boris Zbarsky [:bz] 2007-10-03 19:35:14 PDT
Comment on attachment 283290 [details] [diff] [review]
wip5 (branch 1.8)

sr=bzbarsky
Comment 28 Mats Palmgren (vacation) 2007-10-03 23:35:20 PDT
MOZILLA_1_8_BRANCH
mozilla/content/base/src/nsCommentNode.cpp 	3.72.8.4
mozilla/content/base/src/nsDOMDocumentType.cpp 	1.27.20.5
mozilla/content/base/src/nsGenericDOMDataNode.cpp 	3.156.2.9
mozilla/content/base/src/nsGenericDOMDataNode.h 	3.98.8.9
mozilla/content/base/src/nsTextNode.cpp 	3.49.8.4
mozilla/content/xml/content/src/nsXMLCDATASection.cpp 	1.39.4.4
mozilla/content/xml/content/src/nsXMLProcessingInstruction.cpp 	1.60.8.5

MOZILLA_1_8_0_BRANCH
mozilla/content/base/src/nsCommentNode.cpp 	3.72.14.2
mozilla/content/base/src/nsDOMDocumentType.cpp 	1.27.26.2
mozilla/content/base/src/nsGenericDOMDataNode.cpp 	3.156.8.4
mozilla/content/base/src/nsGenericDOMDataNode.h 	3.98.14.3
mozilla/content/base/src/nsTextNode.cpp 	3.49.14.2
mozilla/content/xml/content/src/nsXMLCDATASection.cpp 	1.39.10.2
mozilla/content/xml/content/src/nsXMLProcessingInstruction.cpp 	1.60.14.2
mozilla/content/xml/content/src/nsXMLStylesheetPI.cpp 	1.12.14.2

-> FIXED
Comment 29 Martijn Wargers [:mwargers] (not working for Mozilla) 2007-10-04 15:32:26 PDT
Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.8pre) Gecko/20071004 BonEcho/2.0.0.8pre

I can see the crash in Firefox2.0.0.7.
Comment 30 Boris Zbarsky [:bz] 2007-11-10 01:03:53 PST
This broke Find code on branch.  See bug 403090.
Comment 31 Al Billings [:abillings] 2007-12-10 17:12:58 PST
Verified in Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.0.14pre) Gecko/20071210 Firefox/1.5.0.13pre. 

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