Open Bug 947044 Opened 6 years ago Updated Last year

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

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set

Tracking

()

REOPENED

People

(Reporter: ejpbruel, Unassigned)

References

(Depends on 1 open bug)

Details

Attachments

(12 obsolete files)

No description provided.
Attached patch patch (obsolete) — Splinter Review
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?
Attachment #8343472 - Flags: review?(luke)
Bug 944930
Depends on: 944930
I like the idea.  This changes the visible "UI" of JS, so might want to see what jorendorff thinks.
Of course this is fine. Let's see it!
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.
Attachment #8343472 - Flags: review?(luke) → review+
Talked to Eddy on IRC. Going to take this, rebase, and get this landed :)
Assignee: ejpbruel → nfitzgerald
Status: NEW → ASSIGNED
(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"
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!
Attachment #8343472 - Attachment is obsolete: true
Attachment #8459944 - Flags: review?(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.
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 :(
Attachment #8459944 - Attachment is obsolete: true
Attachment #8459944 - Flags: review?(luke)
Attachment #8461088 - Flags: review?(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;
Attachment #8461088 - Flags: review?(luke) → review+
Can't realistically fix the tests that check ReferenceError's message until I can run mochitests again.
Depends on: 1042921
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.
Attachment #8461088 - Attachment is obsolete: true
Attachment #8463664 - Flags: review+
Rebased.

Try push to find all the tests that are matching on ReferenceError's message: https://tbpl.mozilla.org/?tree=Try&rev=c6f8fec05754
Attachment #8463664 - Attachment is obsolete: true
Attachment #8491080 - Flags: review+
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
Forgot to push the updated part 1 in that try push.

https://tbpl.mozilla.org/?tree=Try&rev=ba049b6c96d1
Fixing the hazard. Carrying over r+.
Attachment #8491080 - Attachment is obsolete: true
Attachment #8491877 - Flags: review+
Fixing my test fixes.
Attachment #8491610 - Attachment is obsolete: true
Ok, I have a good feeling about this one.

https://tbpl.mozilla.org/?tree=Try&rev=d988f724c88f
Attachment #8491878 - Attachment is obsolete: true
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.
Attachment #8493420 - Flags: review?(gavin.sharp)
Adds a previously missing header include that was causing build errors on non-unified builds.

https://tbpl.mozilla.org/?tree=Try&rev=82ea01cfc834
Attachment #8491877 - Attachment is obsolete: true
Attachment #8493850 - Flags: review+
Woops, I needed to include the inline version of the header.

https://tbpl.mozilla.org/?tree=Try&rev=4af5623696d0
Attachment #8493850 - Attachment is obsolete: true
Attachment #8493910 - Flags: review+
Attachment #8493420 - Flags: review?(gavin.sharp) → review+
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
Attachment #8493910 - Attachment is obsolete: true
Attachment #8494140 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/1049cfbf3426
https://hg.mozilla.org/mozilla-central/rev/b08e57dbef4e
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Depends on: 1072671
Depends on: 1072677
Depends on: 1072947
Backed out at Nick's request.
https://hg.mozilla.org/integration/mozilla-inbound/rev/08a6e06b0255
Status: RESOLVED → REOPENED
Flags: in-testsuite+
Resolution: FIXED → ---
Target Milestone: mozilla35 → ---
If you use 'Nan' instead of 'NaN':
"Nan is not defined, did you mean 'open'?"
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.
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!).
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?
> 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.
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.
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.
Attachment #8493420 - Attachment is obsolete: true
Attachment #8494140 - Attachment is obsolete: true
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...
Assignee: nfitzgerald → nobody
You need to log in before you can comment on or make changes to this bug.