Closed
Bug 634373
Opened 14 years ago
Closed 14 years ago
crash [@ nsCSSStyleSheet::WillDirty] using cross_fuzzv3
Categories
(Core :: CSS Parsing and Computation, defect, P3)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla5
Tracking | Status | |
---|---|---|
firefox5 | --- | fixed |
People
(Reporter: eherokles, Assigned: dbaron)
Details
Attachments
(4 files, 2 obsolete files)
(deleted),
text/plain
|
Details | |
(deleted),
text/plain
|
Details | |
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
bzbarsky
:
review+
johnath
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
IΒ΄ve userd cross_fuzzv3 on firefox4b11 32bit windowsxp PRIMARY_PROBLEM_CLASS: NULL_CLASS_PTR_DEREFERENCE
Attachment #512565 -
Attachment mime type: application/octet-stream → text/plain
Updated•14 years ago
|
Component: XUL → Style System (CSS)
QA Contact: xptoolkit.widgets → style-system
![]() |
||
Comment 1•14 years ago
|
||
Hmm. That's crashing at address 0x3c == 60, which is the offset of mComplete in nsCSSStyleSheetInner, suggesting that mInner is null. I don't see how it can be null on trunk, though: it's set in the constructor and never nulled out. David, any ideas?
![]() |
||
Comment 2•14 years ago
|
||
Though.... looks like 60 might also be the offset of mInner in nsCSSStyleSheet (though my count says 56... but it depends on what happens with those PRPackedBools, etc). So it's possible the _sheet_ is null. Perhaps DeclarationChanged is being called with a second arg of PR_TRUE after the sheet has gone away? I don't see anything obvious preventing that from happening.
Assignee | ||
Comment 4•14 years ago
|
||
oh, never mind. I was just skimming for a UA string, and didn't see one.
Assignee | ||
Updated•14 years ago
|
Keywords: testcase-wanted
Assignee | ||
Comment 5•14 years ago
|
||
(In reply to comment #2) > Though.... looks like 60 might also be the offset of mInner in nsCSSStyleSheet > (though my count says 56... but it depends on what happens with those > PRPackedBools, etc). So it's possible the _sheet_ is null. > > Perhaps DeclarationChanged is being called with a second arg of PR_TRUE after > the sheet has gone away? I don't see anything obvious preventing that from > happening. Given the windbg output, it's easy enough to tell for sure: 1. On a Linux box, download http://releases.mozilla.org/pub/mozilla.org/firefox/releases/4.0b11/update/win32/en-US/firefox-4.0b11.complete.mar 2. run the alias unmar-full-update='MAR=~/builds/mozilla-central/obj/firefox-debugopt/dist/host/bin/mar ~/builds/mozilla-central/mozilla/tools/update-packaging/unwrap_full_update.pl' to extract it 3. objdump -Cd xul.dll Which shows the context is: 1027130a: 57 push %edi 1027130b: 8b f8 mov %eax,%edi 1027130d: 8b 47 3c mov 0x3c(%edi),%eax <== CRASH HERE 10271310: 83 78 4c 00 cmpl $0x0,0x4c(%eax) 10271314: 0f 85 ca 27 15 00 jne 0x103c3ae4 1027131a: 33 c0 xor %eax,%eax 1027131c: 5f pop %edi 1027131d: c3 ret hg cat -rFIREFOX_4_0b11_RELEASE nsCSSStyleSheet.cpp | cat -n gives (crash on line 1553): 1550 nsresult 1551 nsCSSStyleSheet::WillDirty() 1552 { 1553 if (!mInner->mComplete) { 1554 // Do nothing 1555 return NS_OK; 1556 } 1557 1558 if (EnsureUniqueInner() == eUniqueInner_CloneFailed) { 1559 return NS_ERROR_OUT_OF_MEMORY; 1560 } 1561 return NS_OK; 1562 } So pretty clearly |this| is null. FWIW, the offsets, for 32-bit, look to me like: nsCSSStyleSheet: 0x00 vtbl. nsIStyleSheet 0x04 vtbl. nsIDOMCSSStyleSheet 0x08 vtbl. nsICSSLoaderObserver 0x0c mRefCnt 0x10 mTitle mData 0x14 mLength 0x18 mFlags 0x1c mMedia 0x20 mNext 0x24 mParent 0x28 mOwnerRule 0x2c mRuleCollection 0x30 mDocument 0x34 mOwningNode 0x38 mDisabled 0x39 mDirty 0x3c mInner ... nsCSSStyleSheetInner: 0x00 mSheets mHdr 0x04 Header: mLength 0x08 Header: mCapacity 0x0c padding for 64-bit alignment (?) 0x10 array item 0 0x2c array item 7 0x30 mSheetURI 0x34 mOriginalSheetURI 0x38 mBaseURI 0x3c mPrincipal 0x40 mOrderedRules 0x44 mNameSpaceMap 0x48 mFirstChild 0x4c mComplete ... which agrees with this.
![]() |
||
Comment 6•14 years ago
|
||
Ah, I missed mRefCnt on the sheet; that's why I was getting the offset wrong there. Yeah, so we have a null sheet. We currently assert this doesn't happen; we should check instead.
IΒ΄ve used cross_fuzz(random seed, no logging) against firefox4pre13b (nightly:23.2.2011), and get the same crash, but it seems to start at another origin. Well, have a look and the new Windbg output. May be it is helpful.
Reporter | ||
Comment 10•14 years ago
|
||
Assignee | ||
Updated•14 years ago
|
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
Assignee | ||
Comment 11•14 years ago
|
||
It was pretty obvious what to do in most cases, except the one directly related to this bug (the StyleRule::DeclarationChanged case). I should really add some tests too.
Attachment #529146 -
Flags: review?(bzbarsky)
![]() |
||
Updated•14 years ago
|
Attachment #529146 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 12•14 years ago
|
||
As I said, I think the ~GroupRule change is actually unneeded -- replaced with an assertion. This also adds tests.
Attachment #529146 -
Attachment is obsolete: true
Attachment #529291 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 13•14 years ago
|
||
And any hint as to how I might make the first and third test show a crash (without the patch) would be appreciated, especially since I *think* the first test ought to be equivalent to the original report.
Assignee | ||
Comment 14•14 years ago
|
||
Oh... I think I can make the rule match something, and then not restyle yet.
Assignee | ||
Comment 15•14 years ago
|
||
Now I have tests for all the cases that show the crash without the patch. I also left in the two tests that don't crash, since they exercise the dom-wrapper-with-no-rule case.
Attachment #529291 -
Attachment is obsolete: true
Attachment #529304 -
Flags: review?(bzbarsky)
Attachment #529291 -
Flags: review?(bzbarsky)
![]() |
||
Comment 16•14 years ago
|
||
Comment on attachment 529304 [details] [diff] [review] patch r=me
Attachment #529304 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 17•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/064d7c5425a6
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
OS: Windows XP → All
Priority: -- → P3
Hardware: x86 → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla6
Assignee | ||
Comment 18•14 years ago
|
||
Comment on attachment 529304 [details] [diff] [review] patch This is a bunch of null-checks (some in old code, some in new-for-animations code) -- so low risk crash fixes that fix crashes that seem somewhat likely to be hit by fuzzers.
Attachment #529304 -
Flags: approval-mozilla-aurora?
Updated•14 years ago
|
Attachment #529304 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 19•14 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/b11d407b1975
status-firefox5:
--- → fixed
Assignee | ||
Updated•14 years ago
|
Target Milestone: mozilla6 → mozilla5
Assignee | ||
Updated•14 years ago
|
Keywords: testcase-wanted
You need to log in
before you can comment on or make changes to this bug.
Description
•