822842 - Crash [@ nsROCSSPrimitiveValue::Reset] after setting padding-left-value (!?)

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Jesse Ruderman]

Attachment: testcase

1. Load the testcase
2. Quit Firefox immediately.

Result: Some subset of the following three symptoms:

###!!! ASSERTION: Null string should never happen: 'mValue.mString', file layout/style/nsROCSSPrimitiveValue.cpp, line 643

###!!! ASSERTION: Null RGBColor should never happen: 'mValue.mColor', file layout/style/nsROCSSPrimitiveValue.cpp, line 655

Null deref [@ nsROCSSPrimitiveValue::Reset]

Maybe a regression from some of the work in bug 821593?

Comment 1

[Jesse Ruderman]

Attachment: stacks

Comment 2

[Boris Zbarsky [:bzbarsky]]

Very likely, yes (or rather the combination of that and bug 798567).  The problem, at first glance, is that nsROCSSPrimitiveValue::Reset does not reset mType.  So if you Reset() twice in a row after starting with a color/uri/rect/counter/attr/string type, you crash.

And you get the reset-twice behavior if you first unlink and then call the destructor, since unlink resets...

Trevor, mind taking a look?

Comment 3

[Trevor Saunders (:tbsaunde)]

(In reply to Boris Zbarsky (:bz) from comment #2)
> Very likely, yes (or rather the combination of that and bug 798567).  The
> problem, at first glance, is that nsROCSSPrimitiveValue::Reset does not
> reset mType.  So if you Reset() twice in a row after starting with a
> color/uri/rect/counter/attr/string type, you crash.

I'd hope that nsMemory::Free() behaves like free() and accepts null without crashing, but this is the issue here.

> And you get the reset-twice behavior if you first unlink and then call the
> destructor, since unlink resets...

yeah, Reset() should just make mType CSS_UNKNOWN so calling it again is a nop.  Can we force a cycle to test this?

> Trevor, mind taking a look?

I already was, thanks for the guess.

Comment 4

[Boris Zbarsky [:bzbarsky]]

nsMemory::Free accepting null might help string/attr/counter.  But the rect and rgbcolor cases both use NS_RELEASE, which is not null-safe.  We could NS_IF_RELEASE, but it seems better to reset mType to me.

Comment 5

[Trevor Saunders (:tbsaunde)]

(In reply to Boris Zbarsky (:bz) from comment #4)
> nsMemory::Free accepting null might help string/attr/counter.  But the rect
> and rgbcolor cases both use NS_RELEASE, which is not null-safe.  We could
> NS_IF_RELEASE, but it seems better to reset mType to me.

agreed, I mostly didn't write a patch doing that already because I wasn't sure if there was a reasonable way to test this.

Comment 6

[Boris Zbarsky [:bzbarsky]]

SpecialPowers should give you the ability to gc/cc which might trigger this.  Just make sure to set an expando on the RGBColor to force cc to be needed to clean it up.

Comment 7

[Trevor Saunders (:tbsaunde)]

Attachment: patch

In theory the test should work, but I can't figure out how to run it so I can't really  test that.  However manually I could nolong reproduce with the given test case.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 693714 [details] [diff] [review]
patch

Hmm.  Is there SpecialPowers in crashtests?

Comment 9

[Trevor Saunders (:tbsaunde)]

(In reply to Boris Zbarsky (:bz) from comment #8)
> Comment on attachment 693714 [details] [diff] [review]
> patch
> 
> Hmm.  Is there SpecialPowers in crashtests?

I got this working, and image/test/crashtests/ownerdiscard.html seems to assume so too, so I think yes.

It turns out my problem was make crashtest TESt_PATH=blah makes the test harness try and use srcdir/<random unicode> as the manifest instead of blah or srcdir/blah.  However TEST_PATH=blah make crashtest works.

Comment 10

[Trevor Saunders (:tbsaunde)]

Comment on attachment 693714 [details] [diff] [review]
patch

>+    x.getRGBColor().blue.d = x;

that should be getRGBColorValue() of course which is fixed locally.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 693714 [details] [diff] [review]
patch

r=me

Comment 12

[Trevor Saunders (:tbsaunde)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/dc7bc91d2e7d

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/dc7bc91d2e7d

Comment 14

[Paul Silaghi, QA [:pauly]]

Reproduced the issue on 2012-12-18-mozilla-central-debug.
Verified fixed on 2013-03-21-mozilla-beta-debug on Mac OS X 10.7.5