868265 - MozNamedAttrMap.removeNamedItem crash

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jesse Ruderman]

Attachment: testcase (requires fuzz extension)

1. Install https://www.squarefree.com/extensions/domFuzzLite3.xpi
2. Load the testcase

Without a GC, removeNamedItem throws.  With a GC, it tries to return null, but crashes because the WebIDL (added in bug 852135) says it can't return null.

The crash is probably a regression from bug 852135, but did the underlying problem exist before then?

Why does the behavior depend on GC?  Is there a GC hazard lurking here?

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

The problem is that the attributes object holds a weak reference to its element, and after the element goes away, removeNamedItem tries to return null. I think it might be better to make attributes hold a strong reference instead.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Attachment: strong ownership, add some skippability and black-bit-propagation

https://tbpl.mozilla.org/?tree=Try&rev=a4fd9911b8a7
Fixed also QI to nsISupports.

nsIAttribute ctor isn't inline anymore so that there isn't need
to #include nsDOMAttributeMap.

Made nsDOMAttributeMap::RemoveNamedItem to throw similarly as the
*NS version of the method. That would have fixed this bug too, but better 
to make things in such way the GC/CCing isn't detectable from web pages.

Added skippability since I've seen AttributeMaps in logs.
BBP could be improved, but added the easy cases.
Crossing fingers that try results are ok :)

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Attachment: +missing include

https://tbpl.mozilla.org/?tree=Try&rev=5d2654e227c1

Comment 4

[Andrew McCreight [:mccr8]]

Comment on attachment 746046 [details] [diff] [review]
strong ownership, add some skippability and black-bit-propagation

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

::: content/base/src/Attr.cpp
@@ +91,5 @@
> +  }
> +NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_END
> +
> +NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_IN_CC_BEGIN(Attr)
> +  return tmp->IsBlackAndDoesNotNeedTracing(static_cast<nsIAttribute*>(tmp));

same question as for nsDOMAttributeMap.cpp

::: content/base/src/nsDOMAttributeMap.cpp
@@ +101,5 @@
> +  }
> +NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_END
> +
> +NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_IN_CC_BEGIN(nsDOMAttributeMap)
> +  return tmp->IsBlackAndDoesNotNeedTracing(tmp);

Why can't you use IsBlack() here?  The wrapper cache is the only thing we trace.  Should we be tracing additional things?  Or is this to handle the case when our wrapper is unpreserved?  Why do you have to use IsBlack in CanSkipThis?

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

I don't have to use IsBlackAndDoesNotNeedTracing(), but it is there for future proof.
IsBlack() is faster in CanSkipThis: https://bugzilla.mozilla.org/show_bug.cgi?id=794694#c4

Comment 6

[Andrew McCreight [:mccr8]]

Ahh makes sense.  Yes, I managed to totally forget about 794694 somehow.  That was clever of me to think of that. ;)

If you could give this a security rating that would be helpful.  I don't understand the security consequences of the existing code.  Also is it just a regression from bug 852135 or probably an existing problem?  Basically, everything Jesse asked in comment 0.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

This is just a regression from bug 852135, but the patch ends up fixing other stuff too.
I'm not sure about rating. Returning null when bindings expect non-null. I think those have been
sg-crit usually.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9263ee696cff

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9263ee696cff