Persona is no longer an option for authentication on BMO. For more details see Persona Deprecated.
Last Comment Bug 758278 - Sweep crossCompartmentWrappers of all compartments, not only collected ones
: Sweep crossCompartmentWrappers of all compartments, not only collected ones
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: Till Schneidereit [:till]
: Jason Orendorff [:jorendorff]
Depends on:
Blocks: 723350
  Show dependency treegraph
Reported: 2012-05-24 10:15 PDT by Till Schneidereit [:till]
Modified: 2012-07-16 07:58 PDT (History)
10 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch (5.33 KB, patch)
2012-05-24 10:15 PDT, Till Schneidereit [:till]
no flags Details | Diff | Splinter Review
patch, v2 (6.09 KB, patch)
2012-05-24 12:56 PDT, Till Schneidereit [:till]
no flags Details | Diff | Splinter Review
patch, v3 (5.31 KB, patch)
2012-05-24 13:07 PDT, Till Schneidereit [:till]
wmccloskey: review+
Details | Diff | Splinter Review
patch, v4, carrying billm's r+ (5.25 KB, patch)
2012-05-27 12:15 PDT, Till Schneidereit [:till]
till: review+
Details | Diff | Splinter Review

Description Till Schneidereit [:till] 2012-05-24 10:15:51 PDT
Created attachment 626861 [details] [diff] [review]

Bug 723350 uncovered a problem with CCWs not being collected if only one of the two affected compartments is GC'd.

This in turn caused the explosive memory usage described in bug 756549.

The attached patch fixes the problem by sweeping CCWs of all compartments, not just the currently collected ones.
Comment 1 Till Schneidereit [:till] 2012-05-24 10:20:57 PDT
try run:
Comment 2 Andrew McCreight [:mccr8] 2012-05-24 10:35:05 PDT
CCWs for strings don't keep alive their target?  Weird.
Comment 3 Bill McCloskey (:billm) 2012-05-24 11:13:44 PDT
Comment on attachment 626861 [details] [diff] [review]

Review of attachment 626861 [details] [diff] [review]:

Thanks, Till. I'm glad this fixed the problem. Could you fix these small issues and post a rebased patch? Terrence changes JSCompartment::sweep yesterday and the cross-compartment wrappers are now swept a little differently. It won't affect what you're doing here, though.

::: js/src/jscompartment.h
@@ +190,5 @@
>      void                         *data;
>      bool                         active;  // GC flag, whether there are active frames
>      js::WrapperMap               crossCompartmentWrappers;
> +    void sweepCrossCompartmentWrappers();

Please put this after the declaration of sweep.

::: js/src/jsscope.cpp
@@ +1183,5 @@
>      return nbase;
>  }
> +/**
> + *  Remove dead wrappers from the table.

Maybe put a longer comment here. Something like "Remove dead wrappers from the table. We must sweep all compartments, since string entries in the crossCompartmentWrappers table are not marked during markCrossCompartmentWrappers."

Also, multi-line comments should have this format (with only one star on the first and last line):
 * Text goes here.
 * Another line.

@@ +1186,5 @@
> +/**
> + *  Remove dead wrappers from the table.
> + **/
> +void
> +JSCompartment::sweepCrossCompartmentWrappers()

This function should go in jscompartment.cpp, right after JSCompartment::sweep. The sweepBaseShapeTable function is here because it's related to shapes, and shape stuff usually goes in jsscope.cpp.
Comment 4 Till Schneidereit [:till] 2012-05-24 12:56:46 PDT
Created attachment 626925 [details] [diff] [review]
patch, v2

Thanks for the quick review, Bill. Attached is a rebased patch with your comments addressed.
Comment 5 Till Schneidereit [:till] 2012-05-24 13:07:49 PDT
Created attachment 626930 [details] [diff] [review]
patch, v3

Whoops, wrong patch.
Comment 6 Bill McCloskey (:billm) 2012-05-24 17:57:03 PDT
Comment on attachment 626930 [details] [diff] [review]
patch, v3

Review of attachment 626930 [details] [diff] [review]:

Thanks. The try results look good to me. I think this is ready.

::: js/src/jscompartment.cpp
@@ +519,5 @@
>      }
>      active = false;
>  }
> +/*

Please put an extra line after the end brace.
Comment 7 Till Schneidereit [:till] 2012-05-27 12:15:38 PDT
Created attachment 627582 [details] [diff] [review]
patch, v4, carrying billm's r+

Rebased patch with last nit addressed.
Comment 8 Ryan VanderMeulen [:RyanVM] 2012-05-27 14:00:20 PDT
Comment 9 Ryan VanderMeulen [:RyanVM] 2012-05-27 18:48:36 PDT

Note You need to log in before you can comment on or make changes to this bug.