Closed Bug 1223078 Opened 6 years ago Closed 6 years ago

Release wrappers whose weakly held JSObject has died eagerly

Categories

(Core :: XPConnect, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: terrence, Assigned: terrence)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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#.
Attachment #8684990 - Flags: review?(continuation)
Component: JavaScript: GC → XPConnect
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.
Attachment #8684990 - Flags: review?(continuation) → review+
(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!
https://hg.mozilla.org/mozilla-central/rev/e80adf2d71b9
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.