1223078 - Release wrappers whose weakly held JSObject has died eagerly

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Terrence Cole [:terrence]]

Attachment: 00_release_wrapped_js_locally-v0.diff

There is a comment at the top of JSObject2WrappedJSMap::UpdateWeakPointersAfterGC that seems to be total nonsense. It says we must release after the GC, but in practice the release gets called not at the end of the GC, or even at the end of a slice, but just at the end of the uninterruptable beginSweepingZoneGroups where we queued the wrappers for deletion in the first place. So I can't really see any reason whatsoever not to just do the releases immediately.

3 files changed, 6 insertions(+), 35 deletions(-)

Will send this off to try as soon as I have a bug#.

Comment 1

[Terrence Cole [:terrence]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=596a74814ad1

Comment 2

[Andrew McCreight [:mccr8]]

Comment on attachment 8684990 [details] [diff] [review]
00_release_wrapped_js_locally-v0.diff

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

::: js/xpconnect/src/XPCMaps.cpp
@@ +87,5 @@
>  void
>  JSObject2WrappedJSMap::UpdateWeakPointersAfterGC(XPCJSRuntime* runtime)
>  {
>      // Check all wrappers and update their JSObject pointer if it has been
> +    // moved. Release any wrappers who's weakly held JSObject has died.

nit: who's -> whose

@@ +92,2 @@
>  
> +    nsTArray<nsXPCWrappedJS*> dying;

Please change the type of this to |nsTArray<nsRefPtr<nsXPCWrappedJS>>|, then do |dying.AppendElement(dont_AddRef(wrapper)| below. Then you can get rid of the manual release at the end of the method.

Comment 3

[Terrence Cole [:terrence]]

(In reply to Andrew McCreight [:mccr8] from comment #2)
> Comment on attachment 8684990 [details] [diff] [review]
> 00_release_wrapped_js_locally-v0.diff
> 
> Review of attachment 8684990 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +92,2 @@
> >  
> > +    nsTArray<nsXPCWrappedJS*> dying;
> 
> Please change the type of this to |nsTArray<nsRefPtr<nsXPCWrappedJS>>|, then
> do |dying.AppendElement(dont_AddRef(wrapper)| below. Then you can get rid of
> the manual release at the end of the method.

Neat!

Comment 4

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e80adf2d71b9a2dea37fbab22e6be2e1da173d50
Bug 1223078 - Release WrappedJS eagerly; r=mccr8

Comment 5

[Carsten Book [:Tomcat]]

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