1448582 - Assertion failure: !atom_, at js/src/vm/JSFunction.h

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Bob Clary [:bc] (inactive)]

Attachment: atom.txt

1. https://data2.marshadow.io/assets/adserver.php?w=300&h=250&app_code=r3a21df&host=actu.orange.fr&browser=Firefox&plugin_key=Vu%24%24oXGJ:SPUWz:GSq7PGw&version=1&v=2&ifradid=759659861136630

plus 3 others. Recent regression since about 3/22?

2. Assertion failure: !atom_, at /builds/worker/workspace/build/src/js/src/vm/JSFunction.h:373

#01: js::UnixExceptionHandler(int, siginfo*, void*) (/mozilla/builds/nightly/mozilla/firefox-debug/dist/bin/libxul.so)
#02: WasmFaultHandler(int, siginfo*, void*) (/mozilla/builds/nightly/mozilla/firefox-debug/dist/bin/libxul.so)
#03: __restore_rt (sigaction.c:?)
#04: js::SetFunctionNameIfNoOwnName(JSContext*, JS::Handle<JSFunction*>, JS::Handle<JS::Value>, FunctionPrefixKind) (/mozilla/builds/nightly/mozilla/firefox-debug/dist/bin/libxul.so)
#05: Interpret(JSContext*, js::RunState&) (/mozilla/builds/nightly/mozilla/firefox-debug/dist/bin/libxul.so)


JSAtom* displayAtom() const {
        return atom_;
    }

    void setInferredName(JSAtom* atom) {
=>        MOZ_ASSERT(!atom_);
        MOZ_ASSERT(atom);
        MOZ_ASSERT(!hasGuessedAtom());
        setAtom(atom);
        flags_ |= HAS_INFERRED_NAME;
    }

Comment 1

[Jan de Mooij [:jandem]]

anba, regression from your changes maybe?

Comment 2

[André Bargull [:anba]]

Yes, it's a regression from bug 1371591.

I was a bit tricky to find out what exactly went wrong here, because the stack trace isn't that helpful and the link in comment #0 only links to a js-file without clear STR. At least one possible way to trigger this assertion, is to clone a function marked as singleton when the singleton has an inferred name. But I don't know if that's exactly the one case which triggered the assertion in comment #0, because the report lacks STR. (To avoid any misunderstandings, I don't the blame report because it lacks STR, it's only a bit unsatisfactory when I'm not able to pinpoint the exact cause of error! :-)

Comment 3

[Bob Clary [:bc] (inactive)]

fwiw, when I simply list the url in the str it means that it will crash on load. since this is an assertion failure, I didn't explicitly say that a debug build was required. I did neglect to give the os it reproduced on (linux) or the explicit branch sorry.

Comment 4

[André Bargull [:anba]]

Attachment: bug1448582.patch

Clear any existing inferred name in SetFunctionNameIfNoOwnName() which can happen for cloned singleton functions.

This also uncovered another pre-existing issue in NewFunctionClone() where we copied the RESOLVED_LENGTH and RESOLVED_NAME flags to cloned function.

Comment 5

[Jason Orendorff [:jorendorff]]

Comment on attachment 8962330 [details] [diff] [review]
bug1448582.patch

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

Ugh. I should have caught this on review, but I forgot this existed.

::: js/src/vm/JSFunction.cpp
@@ +2395,5 @@
>      if (!funNameAtom)
>          return false;
>  
> +    // An inferred name may already be set if this function is a clone of a
> +    // singleton function.

The current comment on JSFunction::atom_ is unhelpful. Please change it to tell which flags affect the meaning, at least!

This fix creates a weird case where we have a function object with an inferred name and HAS_GUESSED_ATOM flag that are wrong for a moment, unobservably, until we reach the SETFUNNAME instruction. Is that right?

If so, this needs a comment somewhere. I guess in NewFunctionClone, where the wrongness begins.

Consider fixing this in NewFunctionClone instead, so that the function object is never wrong. If you're worried about the performance cost of the extra branch, it could be fixed in CloneFunctionAndScript, right? We're already in the slow path there. r=me whether you change this or not.

Comment 6

[Jason Orendorff [:jorendorff]]

> This fix creates a weird case...

Correction -- the weird case was created in the regressing patch, not this fix.

Comment 7

[André Bargull [:anba]]

> This fix creates a weird case where we have a function object with an inferred name and HAS_GUESSED_ATOM flag that are wrong for a moment, unobservably, until we reach the SETFUNNAME instruction. Is that right?

HAS_GUESSED_ATOM is only set through JSFunction::setGuessedAtom(...), which is only called from [1]. And [1] ensures that setGuessedAtom(...) is never called for functions which get an inferred name, so we can't have an inferred name and HAS_GUESSED_ATOM flag set at the same time. Did you mean HAS_INFERRED_NAME instead of HAS_GUESSED_ATOM?

https://searchfox.org/mozilla-central/rev/003262ae12ce937950ffb8d3b0fa520d1cc38bff/js/src/frontend/NameFunctions.cpp#282-285

> Consider fixing this in NewFunctionClone instead, so that the function object is never wrong. If you're worried about the performance cost of the extra branch, it could be fixed in CloneFunctionAndScript, right? We're already in the slow path there. r=me whether you change this or not.

I assume you mean to handle the case when HAS_INFERRED_NAME is set in NewFunctionClone and then clear the inferred name when cloning the object? That won't work because HAS_INFERRED_NAME is set for compile-time known and dynamically computed inferred names:
---
var o = {
  a: function(){}, // <- Has HAS_INFERRED_NAME set at compile-time.
  ["b"]: function(){}, // <- HAS_INFERRED_NAME is set at runtime in SetFunctionNameIfNoOwnName.
};
---

So when we're in NewFunctionClone and HAS_INFERRED_NAME is set, we don't know which of the two cases apply. And therefore we need to defer the fix-up until we reach SetFunctionNameIfNoOwnName.

Comment 8

[André Bargull [:anba]]

(In reply to Bob Clary [:bc:] from comment #3)
> fwiw, when I simply list the url in the str it means that it will crash on load.

Ah, I see. I was misled because my default browser profile directly closes the tab when I click on the link (probably because of tracking protection, JS not enabled, ad-blocker or the combination of those). When I opened the link in a debug build with a new profile, the tab wasn't directly closed and I also didn't get any assertions, so I guess the cloned singleton function theory was correct.

Comment 9

[Jason Orendorff [:jorendorff]]

> So when we're in NewFunctionClone and HAS_INFERRED_NAME is set, we don't know which of the two cases apply. And therefore we need to defer the fix-up until we reach SetFunctionNameIfNoOwnName.

Ew. :(

Another possibility is to inhibit the optimization for functions which are subject to SETFUNNAME, but that's a bit much.

Or distinct bits for HAS_INFERRED_NAME and HAS_SETFUNNAME. But that would be about as much complexity as the momentary wrongness I want to avoid.  :-P

OK, I guess it just has to be a comment (both on the field that can be momentarily wrong, and in NewFunctionClone, please).

Comment 10

[André Bargull [:anba]]

> Or distinct bits for HAS_INFERRED_NAME and HAS_SETFUNNAME. But that would be
> about as much complexity as the momentary wrongness I want to avoid.  :-P

Thankfully we don't have any space left for additional function flags. :-)

Comment 11

[André Bargull [:anba]]

Attachment: bug1448582.patch

Updated patch to add comments. 

And while adding the new comments I realised that SetFunctionNameIfNoOwnName() actually needs to call clearInferredName() at the top, for the case of class constructors which are marked as singletons and have a dynamically assigned name and an inferred name. See the test case js/src/jit-test/tests/auto-regress/bug1448582-6.js.

Comment 12

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a512f2c1cfc6daddadcc34ea0207bcebd0a95f2

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1fc08acf933
Don't assert when overwriting the atom of cloned singleton functions. r=jorendorff

Comment 14

[Raul Gurzau (:RaulG)]

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