Closed Bug 956156 Opened 6 years ago Closed 6 years ago

Assertion failure: actual->isSubset(frozen), at jsinfer.cpp

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla29
Tracking Status
firefox26 --- unaffected
firefox27 + fixed
firefox28 + fixed
firefox29 + fixed
b2g-v1.2 --- unaffected
b2g-v1.3 --- fixed

People

(Reporter: gkw, Assigned: jandem)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, regression, testcase, Whiteboard: [jsbugmon:update])

Attachments

(3 files, 1 obsolete file)

Attached file stack
function f() {
    ((function g(x) {
        g(x.slice)
    })([]))
}
new f

asserts js debug shell on m-c changeset 25524dc5c99f without any CLI arguments at Assertion failure: actual->isSubset(frozen), at jsinfer.cpp

My configure flags are:

CC="clang -Qunused-arguments" AR=ar CXX="clang++ -Qunused-arguments" sh ./configure --target=x86_64-apple-darwin12.5.0 --enable-optimize --enable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --enable-methodjit --enable-type-inference --disable-tests --with-ccache --enable-threadsafe <other NSPR options>

=== Tinderbox Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20140102085227" and the hash "0764495bc9b8".
The "bad" changeset has the timestamp "20140102085426" and the hash "5c02a8ed40ca".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=0764495bc9b8&tochange=5c02a8ed40ca

Brian, is bug 951497 a likely regressor? Setting s-s because that bug is s-s.
Flags: needinfo?(bhackett1024)
I just have to search "png images" on Google and hit this assert as the resulting page shows up.
Till was hitting this assert with the patch for bug 886193, so I briefly looked into it.

Brian, the problem is that CheckDefinitePropertiesTypeSet can add types to actual, so if the same script is inlined *multiple times*, the actual->isSubset(frozen) assert can fail.
I wonder if FinishCompilation / CheckFrozenTypeSet has a similar issue, but in that case we'll just unnecessarily invalidate the compilation.
FWIW, this is completely unrelated to my patch: It happens if I start a debug build from rev cab31cbee5e6 without any patches applied and go to gmail.
I hit this loading https://maps.google.com/ in a self-built Linux x86-64 debug build from this morning.
This has made debug builds nearly unusable for real browsing; thus nominating for tracking on the releases the regressing patch landed on.
Duplicate of this bug: 956809
Bogus assert, not s-s.
Group: core-security
Attached patch Patch (obsolete) — Splinter Review
Eric, bhackett is out this month so asking you to review this patch. Comment 2 describes the problem. Instead of removing the assert completely, this patch only checks it for the first script. That should still catch most real problems I think without requiring a lot of extra code.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Attachment #8356218 - Flags: review?(efaustbmo)
Comment on attachment 8356218 [details] [diff] [review]
Patch

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

::: js/src/jsinfer.cpp
@@ +1045,5 @@
> +    //
> +    // This function can add types to |actual|, so if a script is inlined more
> +    // than once, |actual| is not necessarily a subset of |frozen|. For now we
> +    // only check this invariant for the first script.
> +    JS_ASSERT_IF(isFirstScript, actual->isSubset(frozen));

Drive-by comment.

I'd rather the initial "actual" were cloned as "firstActual" or whatever, and asserted unconditionally as being subset of frozen of all the frozen |TemporaryTypeSet|s.
(In reply to Shu-yu Guo [:shu] from comment #10)
> Drive-by comment.
> 
> I'd rather the initial "actual" were cloned as "firstActual" or whatever,
> and asserted unconditionally as being subset of frozen of all the frozen
> |TemporaryTypeSet|s.

Yeah but that requires a lot of extra code to do the cloning and store them just for this assert..
(In reply to Jan de Mooij [:jandem] from comment #11)
> (In reply to Shu-yu Guo [:shu] from comment #10)
> > Drive-by comment.
> > 
> > I'd rather the initial "actual" were cloned as "firstActual" or whatever,
> > and asserted unconditionally as being subset of frozen of all the frozen
> > |TemporaryTypeSet|s.
> 
> Yeah but that requires a lot of extra code to do the cloning and store them
> just for this assert..

Maybe a DEBUG-only loop at the top of |types::FinishDefinitePropertiesAnalysis| that does the asserts before any of the copying happens?
Attached patch Patch v2Splinter Review
Alternative fix.
Attachment #8356218 - Attachment is obsolete: true
Attachment #8356218 - Flags: review?(efaustbmo)
Attachment #8357131 - Flags: review?(shu)
Comment on attachment 8357131 [details] [diff] [review]
Patch v2

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

Looks good to me.

::: js/src/jsinfer.cpp
@@ -1062,5 @@
> -        CheckDefinitePropertiesTypeSet(cx, entry.thisTypes, types::TypeScript::ThisTypes(entry.script));
> -        unsigned nargs = entry.script->function() ? entry.script->function()->nargs() : 0;
> -        for (size_t i = 0; i < nargs; i++)
> -            CheckDefinitePropertiesTypeSet(cx, &entry.argTypes[i], types::TypeScript::ArgTypes(entry.script, i));
> -        for (size_t i = 0; i < entry.script->nTypeSets(); i++)

Was this code always using |i| for both the inner and outer loops before??
Attachment #8357131 - Flags: review?(shu) → review+
FYI: I've also hit this on verisign.com.
https://hg.mozilla.org/integration/mozilla-inbound/rev/1956aaafa45a

(In reply to Shu-yu Guo [:shu] from comment #14)
> Was this code always using |i| for both the inner and outer loops before??

Yup, too bad we don't use -Wshadow.
Comment on attachment 8357131 [details] [diff] [review]
Patch v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 951497.
User impact if declined: Frequent asserts in debug builds, makes debugging other problems hard/impossible.
Testing completed (on m-c, etc.): On m-i.
Risk to taking this patch (and alternatives if risky): None, debug-only code.
String or IDL/UUID changes made by this patch: None.
Attachment #8357131 - Flags: approval-mozilla-beta?
Attachment #8357131 - Flags: approval-mozilla-aurora?
Attachment #8357131 - Flags: approval-mozilla-beta?
Attachment #8357131 - Flags: approval-mozilla-beta+
Attachment #8357131 - Flags: approval-mozilla-aurora?
Attachment #8357131 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/1956aaafa45a
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.