947044 - Provide a suggestion when throwing ReferenceError: <name> is not defined

Status: REOPENED

(Core :: JavaScript Engine, enhancement, P5)

Comment 1

[Eddy Bruel [:ejpbruel]]

Attachment: patch

I actually managed to make none of the JIT tests fail, so skipping feedback and putting the patch up for review right away.

Note that this patch depends on bhackett's patch so we can remove the assertion in InnermostScopeObject. What was the bug number for that patch again?

Comment 2

[Luke Wagner [:luke]]

Bug 944930

Comment 3

[Luke Wagner [:luke]]

I like the idea.  This changes the visible "UI" of JS, so might want to see what jorendorff thinks.

Comment 4

[Jason Orendorff [:jorendorff]]

Of course this is fine. Let's see it!

Comment 5

[Luke Wagner [:luke]]

Comment on attachment 8343472 [details] [diff] [review]
patch

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

This looks nice.  My main concern is that we accidentally introduce a source of 1s pauses in pages with reference errors so I've suggested the addition of two sanity limits below.

::: js/src/jscntxt.cpp
@@ +919,5 @@
> +    const size_t n = atom2->length();
> +    const jschar *a = atom1->chars();
> +    const jschar *b = atom2->chars();
> +
> +    size_t *d = cx->pod_malloc<size_t>((m + 1) * (n + 1));

Can you use a Vector<size_t> instead?  Also, could you put a few sanity size limits on m and n.  Given mobile considerations, I'd suggest something low like 1000 or 10,000 (which is still pretty generous for name length).

@@ +950,5 @@
> +             * If the i-length prefix of a and the j-length prefix of b are
> +             * equal in their last character, their edit distance equals that
> +             * of the i-1-length and j-1-length prefix of a and b, respectively.
> +             */
> +            if (a[i - 1] == b[j - 1])

I didn't really study the algorithm, just the iterative impl on wikipedia which I think you've converted directly.  Why is this condition a[i - 1] == b[j - 1] instead of a[i] == b[j]?

@@ +988,5 @@
> +     * were trying to find.
> +     */
> +    AutoIdVector ids(cx);
> +    for (StaticScopeIter<NoGC> ssi(InnermostStaticScope(script, pc ));
> +         !ssi.done(); ssi++)

This should all fit on one 99-char line.

@@ +1017,5 @@
> +            ids.append(NameToId(ssi.lambdaName()));
> +            break;
> +        }
> +    }
> +    if (!GetPropertyNames(cx, cx->global(), JSITER_OWNONLY, &ids)) {

I was thinking: the global object has a ton of properties, but maybe we only care about the 'var' variables.  You could approximate this by filtering on only the JSPROP_PERMANENT properties.

@@ +1025,5 @@
> +    }
> +
> +    RootedAtom bestMatch(cx);
> +    size_t minDistance = (size_t) -1;
> +    for (size_t i = 0; i < ids.length(); ++i) {

In pathological cases, there could be tons of names (esp. on the global object).  How about a sanity limit to limit the max pause time?  I'm not sure how fast this algorithm is in practice, but again I'd be conservative with something like 1000 bindings.  Feel free to measure this in practice to justify a higher limit.

@@ -1252,5 @@
>  }
> -
> -static bool
> -IsJITBrokenHere()
> -{

I assume this change wasn't intended.

Comment 6

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

Talked to Eddy on IRC. Going to take this, rebase, and get this landed :)

Comment 7

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

(In reply to Luke Wagner [:luke] from comment #5)
> @@ +950,5 @@
> > +             * If the i-length prefix of a and the j-length prefix of b are
> > +             * equal in their last character, their edit distance equals that
> > +             * of the i-1-length and j-1-length prefix of a and b, respectively.
> > +             */
> > +            if (a[i - 1] == b[j - 1])
> 
> I didn't really study the algorithm, just the iterative impl on wikipedia
> which I think you've converted directly.  Why is this condition a[i - 1] ==
> b[j - 1] instead of a[i] == b[j]?

From that section of the article: "Note: This section uses 1-based strings instead of 0-based strings"

Comment 8

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

Attachment: reference-error-suggest-name.patch

Rebased and with previous review comments addressed.

Also added a jit-test specifically for the name suggesting.

Haven't fixed tests in the rest of m-c that check ReferenceError message values (unfortunately, it seems we do this a bunch in the debugger).

Also, with the recent changes regarding strings, getting the chars for the atoms now causes an assertion to fail and I'm not sure what the proper way to fix it is:

Triggered from the calls to chars in ComputeEditDistance here:

        const jschar *a = atom1->chars<jschar>(noGC);
        const jschar *b = atom2->chars<jschar>(noGC);

The hasTwoButeChars assertion fails in this method:

MOZ_ALWAYS_INLINE const jschar *
JSLinearString::rawTwoByteChars() const
{
    JS_ASSERT(JSString::isLinear());
    JS_ASSERT(hasTwoByteChars());
    return isInline() ? d.inlineStorageTwoByte : d.s.u2.nonInlineCharsTwoByte;
}

Thanks for the help!

Comment 9

[Luke Wagner [:luke]]

(In reply to Nick Fitzgerald [:fitzgen] from comment #8)
> Also, with the recent changes regarding strings, getting the chars for the
> atoms now causes an assertion to fail and I'm not sure what the proper way
> to fix it is:

Basically, a string can be in either a "Latin1" or "TwoByte" state and this determines which of chars<Latin1>/chars<jschar> you can call.  Usually this is handled by branching on str->hasLatin1Chars() and then calling a template parameterized on CharT.  Alternatively, since performance isn't really an issue here, you could create a AutoStableStringChars and call initTwoByte() which will inflate the string if latin1.  As an added benefit, the jschar* will be stable across GCs, though you don't need this property in ComputeEditDistance.

Comment 10

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

Attachment: reference-error-suggest-name.patch

Ok this passes all the shell tests, but due to bug 1042921, I haven't been able to fix the devtools tests that hard code expected ReferenceError messages :(

Comment 11

[Luke Wagner [:luke]]

Comment on attachment 8461088 [details] [diff] [review]
reference-error-suggest-name.patch

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

Sorry for the delay, looks good!  A few requests:

::: js/src/jscntxt.cpp
@@ +880,5 @@
>          onError(cx, message, reportp);
>  }
>  
> +static const size_t MAX_NAME_LENGTH_FOR_EDIT_DISTANCE = 1000;
> +static const size_t MAX_REFERENCE_ERROR_NAMES_TO_CHECK = 1000;

It's good that you have these limits.  I'm a bit scared about the pathological cases, though, as the web tends to find them.  One case I'm worried about is programmatic name-probing:
  for (n of names)
    try {
       let nameVal = eval(n + ";");
       ...
    } catch (e) {}
Now these guys deserve to be slow, but I'd like avoid introducing serious user-visible jank with this patch since, in the end, it'll be our "fault".  Do you suppose you could measure and tweak the constants to ensure that, on phone-level hardware, the worst-case search takes less than, say, 1/2 second?  I'm thinking 1000*1000*1000 might cross that threshold.

@@ +984,5 @@
> +     * We then pick the name with the shortest edit distance from the name we
> +     * were trying to find.
> +     */
> +    AutoIdVector ids(cx);
> +    for (StaticScopeIter<NoGC> ssi(InnermostStaticScope(script, pc )); !ssi.done(); ssi++) {

nit: no space after 'pc'

@@ +1039,5 @@
> +        return;
> +    }
> +
> +    JSAutoByteString bytes1(cx, atom);
> +    JSAutoByteString bytes2(cx, bestMatch);

Need to check for OOM of these two:
  if (!bytes1.ptr() || !bytes2.ptr())
      return;

Comment 12

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

Can't realistically fix the tests that check ReferenceError's message until I can run mochitests again.

Comment 13

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

Attachment: reference-error-suggest-name.patch

Updated with requested changes.

Takes 26ms to check 1000 reference errors on a couple year old MBP. Shouldn't be too bad on dinky fxos devices either.

Comment 14

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

Attachment: reference-error-suggest-name.patch

Rebased.

Try push to find all the tests that are matching on ReferenceError's message: https://tbpl.mozilla.org/?tree=Try&rev=c6f8fec05754

Comment 15

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

Attachment: reference-error-suggest-name-test-fixes.patch

These are the test fixes for the failures I got in the last try push. Will wait till I get a green push before asking for review on these.

The general approach I took when possible was to prefer checking that a ReferenceError's message contains "<foo> is not defined" rather than matching the whole message exactly and including the suggestion. We might tweak the suggestion algorithm in the future and it would suck to have to update all these tests /again/. However, in some tests I compromised when it was easier to just match the whole message (due to tests using declarative test helpers rather than calling assertion functions directly).

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

Comment 16

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

Forgot to push the updated part 1 in that try push.

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

Comment 17

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

Attachment: reference-error-suggest-name.patch

Fixing the hazard. Carrying over r+.

Comment 18

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

Attachment: reference-error-suggest-name-test-fixes.patch

Fixing my test fixes.

Comment 19

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

New try push: https://tbpl.mozilla.org/?tree=Try&rev=ca162da7e1c5

Comment 20

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

Attachment: reference-error-suggest-name-part-2.patch

Ok, I have a good feeling about this one.

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

Comment 21

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

Comment on attachment 8493420 [details] [diff] [review]
reference-error-suggest-name-part-2.patch

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

Gavin, could you review these cross-tree test fixes? See comment 15 for a description of my approach to the test fixes.

Comment 22

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

Attachment: reference-error-suggest-name.patch

Adds a previously missing header include that was causing build errors on non-unified builds.

https://tbpl.mozilla.org/?tree=Try&rev=82ea01cfc834

Comment 23

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

Attachment: reference-error-suggest-name.patch

Woops, I needed to include the inline version of the header.

https://tbpl.mozilla.org/?tree=Try&rev=4af5623696d0

Comment 24

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

Attachment: reference-error-suggest-name.patch

Sigh... and then I forgot to sort the inline header correctly so the style checker started failing.

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

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1049cfbf3426
https://hg.mozilla.org/integration/mozilla-inbound/rev/b08e57dbef4e

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1049cfbf3426
https://hg.mozilla.org/mozilla-central/rev/b08e57dbef4e

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Backed out at Nick's request.
https://hg.mozilla.org/integration/mozilla-inbound/rev/08a6e06b0255

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/08a6e06b0255

Comment 29

[Tom S [:evilpie]]

If you use 'Nan' instead of 'NaN':
"Nan is not defined, did you mean 'open'?"

Comment 30

[Luke Wagner [:luke]]

So I scanned the patch and there is very nice RAII use so I don't see any leaks.

Here's my theory: when there is a ReferenceError, this patch calls GetPropertyNames on object on the scope chain and this causes us to, in the impl of GetPropertyNames, enumerate the global object.  We have an important memory optimization that tries to be lazy when creating standard-library things on the global object, waiting until the property is first used.  However, enumeration causes everything to be eagerly created so, e.g., this might cause the whole (rather large) Intl library to be created for every window that ever has a single reference error.

Comment 31

[Boris Zbarsky [:bzbarsky]]

Even without the intl stuff, enumerating the global will fault in all the DOM prototypes.  I just tried that in an about:blank document.  It's close to 1.5MB of JS heap usage to do that (about 700KB of Functions, and the various shapes for the DOM protos; for example Window and CSS2Properties both have 50KB of Shapes!).

Comment 32

[Luke Wagner [:luke]]

bz: I wonder if we hit this in content when some unlucky soul for-in's the window object.  Could we have an optimization that allowed us to just collect names (into an array) without mutating the global object?

Comment 33

[Boris Zbarsky [:bzbarsky]]

> I wonder if we hit this in content when some unlucky soul for-in's the window object.

Sure thing.

> Could we have an optimization that allowed us to just collect names (into an array)
> without mutating the global object?

We could, for the specific case of for-in, by using NewEnumerate instead of Enumerate, right?

It would mean a lot more complexity, though, and I expect bugs because afaict NewEnumerate doesn't allow sanely enumerating page-set (whatever _that_ means in a world where properties get lazily resolved) properties.

Comment 34

[Luke Wagner [:luke]]

I'm guessing something like that is what we'd need for this ReferenceError feature (assuming we want builtin constructors to show up in the choices).  If it's a lot of work, it's probably not worth it just for this feature, but perhaps we could motivate the work by showing that many real world pages end up doing a full global enumeration and thus the optimization would be a general memory savings.

Comment 35

[Boris Zbarsky [:bzbarsky]]

This feature doesn't necessarily have the same constraints as actual enumeration does (stable order, uniqueness), so it might be simpler to do something that addresses just this feature.

By the way, it's easy to test whether it's the DOM prototypes are what's eating up memory here.  Change nsGlobalWindow::GetOwnPropertyNames to return without doing anything and see what happens to the memory usage with this patch.

Comment 36

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

If anyone wants to bring this back from the dead in the future: perhaps we can enable this feature only if the global is a debuggee.

But then we are introducing webcontent-visible changes when devtools are open, so maybe that isn't such a great idea...