794694 - Make sure to trace all the gray GCthings, not only wrapper

Status: RESOLVED FIXED

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

Description

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

Attachment: WIP

Still thinking some more generic approach.

Comment 1

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

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


(Haven't tested the patch at all yet. FF starts and closes without leaks.)

Comment 2

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

Attachment: test

Comment 3

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

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

nsGlobalWindow would might need something similar, but it seems to
not trace js stuff during traverse, so it has all different problems.
Fixing that all needs a new bug.

Comment 4

[Andrew McCreight [:mccr8]]

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

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

::: content/base/src/nsXMLHttpRequest.cpp
@@ +534,5 @@
>  NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_IN_CC_END
>  
>  NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_THIS_BEGIN(nsXMLHttpRequest)
> +  return tmp->
> +    IsBlackAndDoesNotNeedTracing(static_cast<nsDOMEventTargetHelper*>(tmp));

see my comment for CanSkipThis in nsDOMEventTargetHelper.

::: content/events/src/nsDOMEventTargetHelper.cpp
@@ +57,4 @@
>  NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_IN_CC_END
>  
>  NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_THIS_BEGIN(nsDOMEventTargetHelper)
> +  return tmp->IsBlackAndDoesNotNeedTracing(tmp);

You don't really need to change this for CanSkipThis.  CanSkipInCC is used for roots, and CanSkipThis is used for children (maybe we should change their names now that this is consistent?).  We only really need to worry about JS holders when we are adding roots. If we later come across the same object as a child, then we know that if it hasn't been added yet, then it must not have any gray JS children. You could add an assertion to this effect I guess. But I don't know how much it matters, except that you will be making things a little slower.

::: dom/base/nsWrapperCacheInlines.h
@@ +24,5 @@
>    return o && !xpc_IsGrayGCThing(o);
>  }
>  
> +static void
> +SearchGray(void* aGCThing, const char* aName, void* aClosure)

CheckParticipatesInCycleCollection in XPCJSRuntime does almost the same thing as this, but I guess it would be hard to share code here.  You should probably check AddToCCKind(js_GetGCThingTraceKind(aGCThing)), though I don't know how much that matters in practice...

Comment 5

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

Attachment: patch

Comment 6

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

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