Closed Bug 794694 Opened 7 years ago Closed 7 years ago

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

Categories

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

x86
All
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: smaug, Assigned: smaug)

Details

Attachments

(3 files)

Attached patch WIPSplinter Review
Still thinking some more generic approach.
https://tbpl.mozilla.org/?tree=Try&rev=f86066e0da03


(Haven't tested the patch at all yet. FF starts and closes without leaks.)
Attached file test
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.
Attachment #665202 - Flags: review?(continuation)
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...
Attachment #665202 - Flags: review?(continuation) → review+
Attached patch patchSplinter Review
https://hg.mozilla.org/mozilla-central/rev/b274e8e3479f
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.