840906 - "Assertion failure: !aIID.Equals(...)" in nsDOMCSSRGBColor::QueryInterface

Status: RESOLVED FIXED

(Core :: DOM: CSS Object Model, defect)

Description

[Jesse Ruderman]

Attachment: testcase (asserts fatally when loaded)

Assertion failure: !aIID.Equals((::nsISupports::COMTypeInfo<int>::kIID)), at ../../../layout/style/nsDOMCSSRGBColor.cpp:37

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Trevor Saunders (:tbsaunde)]

ugh, I'm a clown who shoot himself in the foot with the QI macros again :(

Comment 3

[Boris Zbarsky [:bzbarsky]]

NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY is indeed a footgun.  How about we fix it to not be one?

Comment 4

[Andrew McCreight (out of office until 8/21) [:mccr8]]

What is the problem exactly?  NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY should have gone first?  I'm not familiar with it.

Comment 5

[Boris Zbarsky [:bzbarsky]]

The problem is that NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY totally fails if it's the last thing in the QI, because it does not have a trailing else.

Comment 6

[Alex Keybl [:akeybl]]

Not yet clear why this should track.

What's the user impact?
Did this regress in FF19? Bug 821593 was landed in FF20.

Comment 7

[Boris Zbarsky [:bzbarsky]]

The main impact is lack of fuzzing coverage.   I don't think there should be other user impact here, offhand.

I don't think this is a problem in 19, indeed.

Comment 8

[Trevor Saunders (:tbsaunde)]

Attachment: patch

I could fix the QI but it seems just as easy to nuke it so...

Comment 9

[Boris Zbarsky [:bzbarsky]]

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

I'm not actually sure how non-isupports CC works.  Andrew, can you review this?  If not, I guess we pass it off to Peter?

Comment 10

[Andrew McCreight (out of office until 8/21) [:mccr8]]

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

Review of attachment 713613 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

::: layout/style/nsDOMCSSRGBColor.cpp
@@ +32,4 @@
>  NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_4(nsDOMCSSRGBColor, mAlpha,  mBlue, mGreen, mRed)
> +
> +  NS_IMPL_CYCLE_COLLECTION_ROOT_NATIVE(nsDOMCSSRGBColor, AddRef)
> +  NS_IMPL_CYCLE_COLLECTION_UNROOT_NATIVE(nsDOMCSSRGBColor, Release)

Two extra spaces before both the ROOT and UNROOT lines.

Comment 11

[Jesse Ruderman]

Is there a bug on fixing NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY?

Comment 12

[Trevor Saunders (:tbsaunde)]

(In reply to Jesse Ruderman from comment #11)
> Is there a bug on fixing NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY?

bug 798188?

Comment 13

[Trevor Saunders (:tbsaunde)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8c0f88c09869

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8c0f88c09869

Comment 15

[Lukas Blakk [:lsblakk] use ?needinfo]

Not tracking but would take an uplift if deemed low risk so we can restore coverage.  Please nominate if that's the case.

Comment 16

[Boris Zbarsky [:bzbarsky]]

I think we could do a very low-risk patch here for 20, and should....

Comment 17

[Trevor Saunders (:tbsaunde)]

Attachment: safe patch

Comment 18

[Trevor Saunders (:tbsaunde)]

Comment on attachment 715244 [details] [diff] [review]
safe patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: makes fuzzing harder / lost test coverage
Testing completed (on m-c, etc.): it builds and shouldn't effect tests
Risk to taking this patch (and alternatives if risky): should be pretty low
String or UUID changes made by this patch:  none

Comment 19

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 715244 [details] [diff] [review]
safe patch

Approving for aurora uplift, please try to land this before merge tomorrow (tues) morning PT - otherwise this will need to be approved for beta.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c830a508404d

Comment 21

[Manuela Muntean [Away]]

Issue reproducible with the debug build from 2013-02-13. (assertion failure + Firefox crash)

Verified fixed with Firefox 20 beta 7 debug build, on Mac OSX 10.8.2 (no assertion failure + no Firefox crash)

Build ID: 20130323031722

Comment 22

[Tiziana Sellitto [:tiziana]]

No assertion failure - No crash
Verified fixed on Firefox 21 beta 2 (latest beta debug build), on Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:21.0) Gecko/20130411 Firefox/21.0 

Build ID:  20130411111427