Closed
Bug 1448582
Opened 7 years ago
Closed 7 years ago
Assertion failure: !atom_, at js/src/vm/JSFunction.h
Categories
(Core :: JavaScript Engine, defect, P2)
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox59 | --- | unaffected |
firefox60 | --- | unaffected |
firefox61 | --- | fixed |
People
(Reporter: bc, Assigned: anba)
References
(Regression, )
Details
(Keywords: assertion, regression)
Attachments
(2 files, 1 obsolete file)
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
anba
:
review+
|
Details | Diff | Splinter Review |
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;
}
Reporter | ||
Updated•7 years ago
|
Version: 59 Branch → 61 Branch
Assignee | ||
Comment 2•7 years ago
|
||
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! :-)
Assignee: nobody → andrebargull
Status: NEW → ASSIGNED
Flags: needinfo?(andrebargull)
Reporter | ||
Comment 3•7 years ago
|
||
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.
Assignee | ||
Comment 4•7 years ago
|
||
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.
Attachment #8962330 -
Flags: review?(jorendorff)
Comment 5•7 years ago
|
||
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.
Attachment #8962330 -
Flags: review?(jorendorff) → review+
Comment 6•7 years ago
|
||
> This fix creates a weird case...
Correction -- the weird case was created in the regressing patch, not this fix.
Assignee | ||
Comment 7•7 years ago
|
||
> 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.
Assignee | ||
Comment 8•7 years ago
|
||
(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•7 years ago
|
||
> 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).
Flags: needinfo?(andrebargull)
Assignee | ||
Comment 10•7 years ago
|
||
> 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. :-)
Flags: needinfo?(andrebargull)
Assignee | ||
Comment 11•7 years ago
|
||
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.
Attachment #8962330 -
Attachment is obsolete: true
Attachment #8962866 -
Flags: review+
Updated•7 years ago
|
Priority: -- → P2
Assignee | ||
Comment 12•7 years ago
|
||
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a512f2c1cfc6daddadcc34ea0207bcebd0a95f2
Keywords: checkin-needed
Comment 13•7 years ago
|
||
Pushed by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1fc08acf933
Don't assert when overwriting the atom of cloned singleton functions. r=jorendorff
Keywords: checkin-needed
Comment 14•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Updated•7 years ago
|
status-firefox59:
--- → unaffected
status-firefox-esr52:
--- → unaffected
You need to log in
before you can comment on or make changes to this bug.
Description
•