782467 - Remove JSContext::sharpObjectMap

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

In https://groups.google.com/forum/?fromgroups#!topic/mozilla.dev.tech.js-engine.internals/V0sBu0ZvhXY, Terrence said the following.

> I can't speak for the behavioural side of things, but I think
> sharpObjectMap can go now.  It was not clear that it could go before (I
> cleaned up Object::toSource recently) because the comment explaining
> what it is doing is not terribly clear about what the possible rooting
> scenarios are and the code was (and still largely is) a forest of
> confusion and despair.
>
> After more thought, I can only see one situation it could be there to
> protect us from: a getter prop (running in JS code) which removes two
> very specific elements from the object being to-sourced, then GC's at
> exactly the wrong moment.  If the target of the sharp was on a different
> branch recursion and was therefore not held alive by the stack (and not
> by tracing, since it was removed), but the referring element was kept
> alive because it is on the stack and we are about to trace it (if it
> were alive from tracing we would have cleaned the sharp ref), then we
> could try to toSource a dead object.  At least, I think this is the gist
> of the comment in the source: at least its the only feasible situation
> that I see that would require the sharpObjectMap in any situation.
>
> Now that toSource is purely recursive, I think just using RootedObject
> should be fine. 

Brendan concurred:

> As long as you root everything and check stack depth, should be.

Note that we currently have inconsistent behaviour w.r.t. cyclic objects and toSource(), viz:

  js> let x = {};
  js> x.a = x;
  ({a:{a:{}}})
  js>
  js> let y = []
  js> y[0] = y;
  too much recursion

Comment 1

[Terrence Cole [:terrence]]

And we have tests asserting this inconsistent behavior.  Would it be okay to "fix" this or would it be too much of a behavior change?

Comment 2

[Nicholas Nethercote [inactive]]

(In reply to Terrence Cole [:terrence] from comment #1)
> And we have tests asserting this inconsistent behavior.  Would it be okay to
> "fix" this or would it be too much of a behavior change?

Seems ok to me, esp. since toSource() is non-standard.  The x.a case above is bogus, right?

Comment 3

[Terrence Cole [:terrence]]

Yes, we should also change that behavior.  There is a separate bool in the recursion check for "sharpness" and for "recursed" and how we treat them (again defined by existing tests) is magical in the extreme.  It turns out we only stop when sharpness is set, but we only set sharp after recursed is set.  I don't even.  I just minimized the existing logic.

A fullish rewrite is on offer this time, will have a patch today or tomorrow.

Comment 4

[Terrence Cole [:terrence]]

Attachment: v0

Tests pass and try is looking pretty green:
https://tbpl.mozilla.org/?tree=Try&rev=1081849d1430

Comment 5

[Nicholas Nethercote [inactive]]

Comment on attachment 653005 [details] [diff] [review]
v0

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

I love the amount of code removed by this patch, but I'd be lying if I said I truly understood it.  So rs=me but if you can find someone else who knows this code better than I do to review that'd be a good thing.  Sorry.

::: js/src/tests/js1_5/extensions/regress-369696-01.js
@@ +24,5 @@
>    q.__defineGetter__("0", q.toString);
>    q[2] = q;
>    try
>    {
> +    assertEq(q.toSource(), "[\"\", , []]", "wrong string");

Can you explain this?  I don't understand it, and when I try entering these lines into a JS shell I get "too much recursion" on the |q[2] = q;| line.

Comment 6

[Terrence Cole [:terrence]]

(In reply to Nicholas Nethercote [:njn] from comment #5)
> I love the amount of code removed by this patch, but I'd be lying if I said
> I truly understood it.  So rs=me but if you can find someone else who knows
> this code better than I do to review that'd be a good thing.  Sorry.

No problem, I'll just wait until Waldo gets back next week.
 
> ::: js/src/tests/js1_5/extensions/regress-369696-01.js
> @@ +24,5 @@
> >    q.__defineGetter__("0", q.toString);
> >    q[2] = q;
> >    try
> >    {
> > +    assertEq(q.toSource(), "[\"\", , []]", "wrong string");
> 
> Can you explain this?  I don't understand it, and when I try entering these
> lines into a JS shell I get "too much recursion" on the |q[2] = q;| line.

The prior behavior (encoded in this test explicitly) was to get a "too much recursion" error, so your shell is correct.  Given that this is insane and the toSource feature is non-standard, I've changed this to give... well, whatever it gives now.

With the patched code, we're sharing the cycle detector set for arrays between toString and toSource, so we get ["", , []].  Array.toString for an array we have visited is "" and Array.toSource for same is [].  Our other option is to have one set for toSource and one set for toString, which would yield: [",,", , []].  I did not consider this difference to be worth adding an extra hashset + malloc to every single context.

Comment 7

[Terrence Cole [:terrence]]

Attachment: v0 - rebased

Rebased.

Forgot to post the numbers last time around:
 8 files changed, 116 insertions(+), 426 deletions(-)

Comment 8

[Jeff Walden [:Waldo]]

Comment on attachment 655669 [details] [diff] [review]
v0 - rebased

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

Nice.  I wish I'd had time to do this, or follow up on it, after the initial sharp removal stuff, but too much other work to do... :-\

::: js/src/jscntxt.cpp
@@ +87,5 @@
> +    }
> +}
> +
> +void
> +js::TraceCycleDetectionSet(JSTracer *trc, js::ObjectSet *set)

I am going to pretend I know that this method is correct.

::: js/src/jscntxt.h
@@ +49,5 @@
> +class AutoCycleDetector
> +{
> +    JSContext *cx;
> +    JSObject *obj;
> +    bool cycle;

cyclic might be a better name.

@@ +71,5 @@
>  };
>  
> +/* Updates references in the cycle detection set if the GC moves them. */
> +extern void
> +TraceCycleDetectionSet(JSTracer *trc, js::ObjectSet *set);

Remove the js::.  And pass by reference to emphasize non-nullness?

::: js/src/jsobj.cpp
@@ +139,5 @@
> +
> +    JSString *gsop[2];
> +    SkipRoot skipGsop(cx, &gsop, 2);
> +
> +    JSIdArray *ida = JS_Enumerate(cx, obj);

Use GetOwnPropertyNames for this.  JSIdArray is old and busted, and much slower to boot, because it does a GetOwnPropertyNames, then a copy into the JSIdArray.  (Not that speed matters here, but JSIdArray obfuscation does...)

@@ -425,3 @@
>              bool doGet = true;
>              if (obj2->isNative()) {
> -                Shape *shape = (Shape *) prop;

You sure we don't need this cast any more?

::: js/src/jsprvtd.h
@@ +161,5 @@
>  class BreakpointSite;
>  class Debugger;
>  class WatchpointMap;
>  
> +typedef HashSet<JSObject *> ObjectSet;

Put this in jscntxt.h, please, or in jsobj.h somewhere if you really want to encourage its use past the cycle detection context?

::: js/src/tests/js1_5/extensions/regress-369696-01.js
@@ +33,1 @@
>    }

Get rid of the whole try-catch and just have the first assertEq, at this point.

Comment 9

[Nicholas Nethercote [inactive]]

> > +    bool cycle;
> 
> cyclic might be a better name.

Or |isCyclic|?  /me likes |isAdjective| and |hasNoun| names for booleans.
</bikeshed>

Comment 10

[Terrence Cole [:terrence]]

Green try run at:
https://tbpl.mozilla.org/?tree=Try&rev=1081849d1430

Pushed at:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d8832ad7f1f

Comment 11

[Terrence Cole [:terrence]]

(In reply to Nicholas Nethercote [:njn] from comment #9)
> Or |isCyclic|?  /me likes |isAdjective| and |hasNoun| names for booleans.
> </bikeshed>

Whoops, I didn't see the color change before I pushed!  Feel free to file a new bug if you really want it changed ;-).

Comment 12

[Philipp von Weitershausen [:philikon]]

https://hg.mozilla.org/mozilla-central/rev/7d8832ad7f1f