725446 - BBP for ContentList

Status: RESOLVED FIXED

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

Description

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

Attachment: WIP

Comment 1

[Andrew McCreight [:mccr8]]

Comment on attachment 595490 [details] [diff] [review]
WIP

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

Looks good to me, aside from my one comment.

::: content/base/src/nsContentList.cpp
@@ +93,5 @@
> +NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_BEGIN(nsBaseContentList)
> +  if (nsCCUncollectableMarker::sGeneration && tmp->IsBlack()) {
> +    JSObject* o = tmp->PreservingWrapper() ? 
> +      tmp->GetWrapperPreserveColor() : tmp->GetExpandoObjectPreserveColor();
> +    xpc_UnmarkGrayObject(o);

Didn't we just check that our wrapper is black?  Isn't o going to always be non-gray here?

Comment 2

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

Yes. silly me. I'll change the code to unmark only if !tmp->PreservingWrapper()

Comment 3

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

Er, except that is not needed.

Comment 4

[Andrew McCreight [:mccr8]]

Because if the list is owned by a black wrapper, it won't also have a separate expando object that it owns?

Comment 5

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

Well, it is the black proxy object which has the expando. And that should be be black too.

Comment 6

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

(Need to get some documentation all this preservedwrapper/proxy/expando handling)

Comment 7

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

Attachment: patch

Comment 8

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

(In reply to Olli Pettay [:smaug] from comment #5)
> Well, it is the black proxy object which has the expando. And that should be
> be black too.
Since if something marks the proxy black, its reserved slots get marked too.

Comment 9

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

https://hg.mozilla.org/mozilla-central/rev/445e1040bfcf