1035287 - Crash [@ getClass] or Crash [@ js::ScriptSourceObject::initFromOptions] with enableTrackAllocations

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 1dc6b294800d (run with --fuzzing-safe):


enableTrackAllocations();
function f() {
    eval('f();');
}
f();

Comment 1

[Christian Holler (:decoder)]

Attachment: [crash-signature] Machine-readable crash signature

Comment 2

[Christian Holler (:decoder)]

Needinfo from fitzgen because enableTrackAllocations() is involved.

Comment 3

[Christian Holler (:decoder)]

Another test, involving setObjectMetadataCallback:


function callback() {}
setObjectMetadataCallback(callback);
var d = {
  p: function () {
         with (d) { q(); }
     }
};
d.q = function() { eval('this.p()'); }
d.p();

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

I don't think this is related to SavedStacks.

Comment 5

[Jason Orendorff [:jorendorff]]

Attachment: bug-1035287-v1.patch

There are two halves to this one.

First, the easy part: a missing OOM check.

But with this change, the test exits with exit code 3 *without* reporting "too much recursion". fitzgen says bug 1006876 might be the culprit; no obvious fix.

Comment 6

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Will work on the second part.

Comment 7

[Jim Blandy :jimb]

Comment on attachment 8452802 [details] [diff] [review]
bug-1035287-v1.patch

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

FML

Comment 9

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: saved-stacks-use-iteration.patch

Rolled the testcase into this patch.

This patch rewrites js::SavedStacks::insertFrames to use iteration instead of recursion, which allows us to remove the hacky and buggy CHECK_RECURSION_DONT_REPORT that was causing the recursion error not to be reported in this case, but if it was missing then the error would get reported multiple times in other cases.

Try push w/ all patches: https://tbpl.mozilla.org/?tree=Try&rev=e7b2fb1cabb4

Comment 10

[Jason Orendorff [:jorendorff]]

Comment on attachment 8454918 [details] [diff] [review]
saved-stacks-use-iteration.patch

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

Skimmed it. It looks OK, but that's a lot of code. Bouncing to Jim.

Jim, can you please check Stack.h and the Stack iterator classes and make sure something like FrameState doesn't already exist?

Sigh. The stack seems to have grown a lot of hair. The last time it was redesigned, Luke was doing everything possible to save writes to the stack frame -- which led to complexity. Now that we have Ion, the tradeoffs may have changed. Maybe use cases like this (and Debugger) should be seen as affecting the tradeoffs too.

::: js/src/vm/SavedStacks.h
@@ +148,5 @@
>          PreBarrieredAtom source;
>          size_t           line;
>          size_t           column;
>      };
>      class MOZ_STACK_CLASS AutoLocationValueRooter : public JS::CustomAutoRooter

Pre-existing style nit: Blank line between classes, please. And throughout this file.

Comment 11

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Jason Orendorff [:jorendorff] from comment #10)
> Pre-existing style nit: Blank line between classes, please. And throughout
> this file.

Something changed in the past month with Splinter and it doesn't recognize newlines in my patches for whatever reason. If you inspect the patch manually, or apply it locally, the newlines are there.

Comment 12

[Shu-yu Guo [:shu]]

Comment on attachment 8454918 [details] [diff] [review]
saved-stacks-use-iteration.patch

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

I feel that there are more AutoTRooters here than necessary in the SavedStack stuff. I wonder if we can cut down on the number by at least 1 for code simplicity's sake.

Also Jason, I don't think anything like FrameState exists that keeps just the location information.

::: js/src/vm/SavedStacks.cpp
@@ +496,5 @@
> +    AutoFrameStateVector stackState(cx);
> +    while (!iter.done()) {
> +        AutoFrameStateRooter frameState(cx, FrameState(iter));
> +
> +        AutoLocationValueRooter location(cx);

Seems wasteful to root another LocationValue here, only to copy it inside frameState, which is already rooted. Can getLocation just work with the LocationValue already inside frameState?

Alternatively, you could get the location first so that frameState isn't live across the can-GC getLocation call. Then you can just append a plain stack-allocated FrameState.

@@ +518,5 @@
>      }
> +    // Iterate through |stackState| in reverse order and get or create the
> +    // actual SavedFrame instances.
> +    RootedSavedFrame parentFrame(cx, nullptr);
> +    for (int32_t i = stackState->length() - 1; i >= 0; i--) {

I don't think this is a problem in practice since our stack limit means the max # of frames won't approach close to the int32 limit, but in case you get compiler warnings just change this to

for (size_t i = stackState->length(); i != 0; i--) {
    // index using i-1 in the body
}

@@ +525,5 @@
> +                                            stackState[i].location.line,
> +                                            stackState[i].location.column,
> +                                            stackState[i].name,
> +                                            parentFrame,
> +                                            stackState[i].principals);

The rooting contract between lookup and getOrCreateSavedFrame seems wrong right now.

createFrameFromLookup has lookup alive across can-GC calls, but its type is const Lookup &, which the compiler is free to compile as a copy. Seems to me you need a Handle type for Lookup.

@@ +640,5 @@
> +        if (!locationp->source)
> +            return false;
> +
> +        uint32_t column;
> +        locationp->line = iter.computeLine(&column);

iter.computeLine(&locationp->column)

@@ +668,5 @@
>              return false;
>      }
>      locationp.set(p->value());
>      return true;
>  }

Newline here.

@@ +704,5 @@
> +    name = fs.name;
> +    location = fs.location;
> +
> +    return *this;
> +}

I would implement a move constructor over the operator=

::: js/src/vm/SavedStacks.h
@@ +200,5 @@
>      PCLocationMap pcLocationMap;
>      void sweepPCLocationMap();
> +    bool getLocation(JSContext *cx, const FrameIter &iter, MutableHandleLocationValue locationp);
> +
> +    struct FrameState {

Style nit: { on newline

@@ +211,5 @@
> +
> +        void trace(JSTracer *trc);
> +
> +        JSPrincipals  *principals;
> +        JSAtom        *name;

Would name make sense to be part of LocationValue? Then you wouldn't need the AutoFrameStateRooter class below since all FrameState needs to do is refcount principals. You'd still need AutoFrameStateVector though. Maybe a little simpler, but not by much.

@@ +220,5 @@
> +      public:
> +        AutoFrameStateRooter(JSContext *cx, const FrameState &fs)
> +            : JS::CustomAutoRooter(cx),
> +              value(fs)
> +              { }

Style nit: { } aligned to AutoFrameStateRooter and 2-space indent on : for initializer lists.

@@ +235,5 @@
> +
> +    class MOZ_STACK_CLASS AutoFrameStateVector : public JS::CustomAutoRooter {
> +      public:
> +        AutoFrameStateVector(JSContext *cx)
> +            : JS::CustomAutoRooter(cx),

Style nit: 2-space indent on :

@@ +249,5 @@
> +
> +        virtual void trace(JSTracer *trc) {
> +            for (size_t i = 0, len = frames.length(); i < len; i++) {
> +                frames[i].trace(trc);
> +            }

Style nit: no { } around single-line loops. I also dislike the len = style here and rather that it were just for (size_t i = 0; i < frames.length(); i++)

Comment 13

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Shu-yu Guo [:shu] from comment #12)
> @@ +640,5 @@
> > +        if (!locationp->source)
> > +            return false;
> > +
> > +        uint32_t column;
> > +        locationp->line = iter.computeLine(&column);
> 
> iter.computeLine(&locationp->column)

If I do this, then I get the following error. Is there a better way than making a temporary variable and relying on conversion?

/Users/fitzgen/src/spidermonkey/js/src/vm/SavedStacks.cpp:655:26: error: cannot initialize a parameter of type 'uint32_t *' (aka 'unsigned int *') with an rvalue of type 'size_t *' (aka 'unsigned long *')
        iter.computeLine(&locationp->column);
                         ^~~~~~~~~~~~~~~~~~

> @@ +668,5 @@
> >              return false;
> >      }
> >      locationp.set(p->value());
> >      return true;
> >  }
> 
> Newline here.

See comment 11. A recent change in splinter made it dislike my patches, but Jim also reported earlier today that it's handling them again now.

*Shrug*

Comment 14

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: saved-stacks-use-iteration.patch

https://tbpl.mozilla.org/?tree=Try&rev=17560537c690

* Added HandleLocation
* Removed AutoFrameStateRooter
* Plus other nits and stuff

Comment 15

[Shu-yu Guo [:shu]]

(In reply to Nick Fitzgerald [:fitzgen] from comment #13)
> (In reply to Shu-yu Guo [:shu] from comment #12)
> > @@ +640,5 @@
> > > +        if (!locationp->source)
> > > +            return false;
> > > +
> > > +        uint32_t column;
> > > +        locationp->line = iter.computeLine(&column);
> > 
> > iter.computeLine(&locationp->column)
> 
> If I do this, then I get the following error. Is there a better way than
> making a temporary variable and relying on conversion?
> 
> /Users/fitzgen/src/spidermonkey/js/src/vm/SavedStacks.cpp:655:26: error:
> cannot initialize a parameter of type 'uint32_t *' (aka 'unsigned int *')
> with an rvalue of type 'size_t *' (aka 'unsigned long *')
>         iter.computeLine(&locationp->column);
>                          ^~~~~~~~~~~~~~~~~~

Well, if computeLine always returns a uint32_t, how about change LocationValue.column to also be uint32_t?

Comment 16

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8459082 [details] [diff] [review]
saved-stacks-use-iteration.patch

Moving review to Eddy because Shu is going on PTO and then to a conference.

Comment 17

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Shu-yu Guo [:shu] from comment #15)
> (In reply to Nick Fitzgerald [:fitzgen] from comment #13)
> > (In reply to Shu-yu Guo [:shu] from comment #12)
> > > @@ +640,5 @@
> > > > +        if (!locationp->source)
> > > > +            return false;
> > > > +
> > > > +        uint32_t column;
> > > > +        locationp->line = iter.computeLine(&column);
> > > 
> > > iter.computeLine(&locationp->column)
> > 
> > If I do this, then I get the following error. Is there a better way than
> > making a temporary variable and relying on conversion?
> > 
> > /Users/fitzgen/src/spidermonkey/js/src/vm/SavedStacks.cpp:655:26: error:
> > cannot initialize a parameter of type 'uint32_t *' (aka 'unsigned int *')
> > with an rvalue of type 'size_t *' (aka 'unsigned long *')
> >         iter.computeLine(&locationp->column);
> >                          ^~~~~~~~~~~~~~~~~~
> 
> Well, if computeLine always returns a uint32_t, how about change
> LocationValue.column to also be uint32_t?

You know, that's just crazy enough to work!

Comment 18

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: saved-stacks-use-iteration.patch

s/size_t/uint32_t/ per discussion above.

Shu says he can review this before leaving on PTO (thanks).

Comment 19

[Shu-yu Guo [:shu]]

Comment on attachment 8459084 [details] [diff] [review]
saved-stacks-use-iteration.patch

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

Looks good.

::: js/src/vm/SavedStacks.cpp
@@ +24,5 @@
>  
>  namespace js {
>  
>  struct SavedFrame::Lookup {
> +    Lookup(JSAtom *source, size_t line, uint32_t column, JSAtom *functionDisplayName,

uint32_t line

@@ +69,5 @@
>  
>      SavedFrame::Lookup value;
>  };
>  
> +class MOZ_NONHEAP_CLASS SavedFrame::HandleLookup

NONHEAP is not wrong here, but you don't need it. JS::Handle is NONHEAP instead of STACK because common Handles are statically allocated in the .data section.

Comment 20

[Jason Orendorff [:jorendorff]]

Landed "part 1" (the missing oom check) to get it out of my queue. +leave-open.
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5b6b138a900

Comment 21

[Ryan VanderMeulen [:RyanVM]]

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

Comment 22

[Christian Holler (:decoder)]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision 4bafe35cfb65).

Comment 23

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: saved-stacks-use-iteration.patch

This should fix that hazard: https://tbpl.mozilla.org/?tree=Try&rev=4be3789af8e4

Comment 24

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

No more hazards!

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing.

Comment 26

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: saved-stacks-use-iteration.patch

Rebased.

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/55b20adc6177

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/55b20adc6177