The default bug view has changed. See this FAQ.

crash [@ nsCSSStyleSheet::WillDirty] using cross_fuzzv3

RESOLVED FIXED in Firefox 5

Status

()

Core
CSS Parsing and Computation
P3
critical
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: eherokles, Assigned: dbaron)

Tracking

Trunk
mozilla5
Points:
---

Firefox Tracking Flags

(firefox5 fixed)

Details

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
Created attachment 512565 [details]
Here is the WinDbg output

I´ve userd cross_fuzzv3 on
firefox4b11 32bit windowsxp

PRIMARY_PROBLEM_CLASS:  NULL_CLASS_PTR_DEREFERENCE
(Reporter)

Updated

6 years ago
Attachment #512565 - Attachment mime type: application/octet-stream → text/plain

Updated

6 years ago
Component: XUL → Style System (CSS)
QA Contact: xptoolkit.widgets → style-system
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?
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 3

6 years ago
What version of Firefox was this in?
(Assignee)

Comment 4

6 years ago
oh, never mind.  I was just skimming for a UA string, and didn't see one.
(Assignee)

Updated

6 years ago
Keywords: testcase-wanted
(Assignee)

Comment 5

6 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.
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.
Or CC sheets + rules and have rules hold strong refs to sheets.
(Reporter)

Comment 8

6 years ago
Created attachment 513102 [details]
Seems to be the same
(Reporter)

Comment 9

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

6 years ago
Created attachment 514569 [details]
The windbg output from nightly 23.2.2011
(Assignee)

Updated

6 years ago
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
(Assignee)

Comment 11

6 years ago
Created attachment 529146 [details] [diff] [review]
audit for null sheet handling

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)
Attachment #529146 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 14

6 years ago
Created attachment 529291 [details] [diff] [review]
audit for null sheet handling

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 15

6 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 16

6 years ago
Oh... I think I can make the rule match something, and then not restyle yet.
(Assignee)

Comment 17

6 years ago
Created attachment 529304 [details] [diff] [review]
patch

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 on attachment 529304 [details] [diff] [review]
patch

r=me
Attachment #529304 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 19

6 years ago
http://hg.mozilla.org/mozilla-central/rev/064d7c5425a6
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
OS: Windows XP → All
Priority: -- → P3
Hardware: x86 → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla6
(Assignee)

Comment 20

6 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?
Attachment #529304 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 21

6 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/b11d407b1975
status-firefox5: --- → fixed
(Assignee)

Updated

6 years ago
Target Milestone: mozilla6 → mozilla5
(Assignee)

Updated

6 years ago
Keywords: testcase-wanted
You need to log in before you can comment on or make changes to this bug.