897507 - Crash in js::frontend::IsIdentifier(JSLinearString*)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Matthew Maxwell]

Attachment: ff-crash.zip

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20100101 Firefox/22.0 (Beta/Release)
Build ID: 20130618035212

Steps to reproduce:

1. Unzip ff-crash to wwwroot.
2. Go to localhost/ff-crash/ff-crash.html.
3. Click the "Create widget" button.


Actual results:

Browser exits and Crash Reporter appears.


Expected results:

The console should show the message "Widget Created." and log an object.

Comment 1

[Colin S]

Crash report is at https://crash-stats.mozilla.com/report/index/ef2db193-2e03-4c21-b57a-98c912130722

Comment 2

[Loic]

Regression range:
good=2013-02-13
bad=2013-02-14
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=161a347bda5b&tochange=aceeea086ccb

Comment 3

[Alice0775 White]

Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/b93da5157462
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20130213 Firefox/21.0 ID:20130213094627
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/872af2305af3
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20130213 Firefox/21.0 ID:20130213101725
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=b93da5157462&tochange=872af2305af3

Regressed by:
985508c04c80	Till Schneidereit — Bug 679940 - Share bytecode, source notes and atoms of functions in a runtime wherever possible. r=bhackett

Comment 4

[Till Schneidereit [:till]]

Matthew, thanks for the report. I can reproduce the crash here and hope to have a fix soon.

Additionally, you might want to look into what's wrong with your code minifier. Widget.js consists of 314 useful characters plus about 4 million semicolons in the middle. I guess that's not really the effect you wanted.

Comment 5

[Matthew Maxwell]

Hi Till,

The semicolons are there on purpose, as the issue only happened with a certain amount of characters before the this.inherited call.  The actual minified file that was causing the initial error is not something I would have been able to provide.

Comment 6

[Till Schneidereit [:till]]

Oh, I see. Thanks for the explanation - that spares me from going on a wild goose chase for bugs related to the semicolons.

Comment 7

[Till Schneidereit [:till]]

Attachment: Debugger dump of corrupted atom

This is all very strange. At the very end of that ~4Mb line in Widget.js, there's a function definition that, pretty-printed, looks like this:

function() {
    this.inherited(arguments);
    this.i18n = {};
    lang.mixin(this.i18n, {
        a: 1
    });
    lang.mixin(this.i18n, {
        b: 1
    });
}

The crash happens during the callprop for `this.inherited()`. More specifically, the atom for `inherited`, stored in script.atoms[0], seems to be corrupted. Using `js_DumpAtom` on it triggers asserts, too. The chars do look very weird:

JSString::dumpChars(atoms[0].get()->d.u1.chars, 3) => "\u9998\u07b4\x01"

Weirdly, just adding a `return` to the end of the function removes the crash.

Attached is a debugger dump of the atom, which I can't really read, I'm afraid. I'll read through the regressing patch again and see if I see anything suspicious. Also, maybe bhackett can say more about this.

@Matthew, thanks for reporting this bug! A workaround you can hopefully use until we have fixed it is to either change the function's content as described above, or by just moving it to the next line, which apparently also stops the crash from happening.

Comment 8

[Till Schneidereit [:till]]

Attachment: part 1: remove functions from header files that are only used in jsscript.cpp

This patch just removes the declarations of SaveSharedScriptData and MarkScriptBytecode from jsscript.h, and moves the latters definition into jsscript.cpp and renames it to MarkScriptData.

Comment 9

[Till Schneidereit [:till]]

Attachment: part 2: add nsrcnotes argument to SaveSharedScriptData and use it instead of script->numNotes()

Turns out script->numNotes() doesn't always report the same number of notes BytecodeEmitter::countFinalSourceNotes() does. The latter overcounts under some circumstances, which aren't entirely clear to me. Given that, after fixing this crash, the only negative implication is that we allocate space for one JSAtom* too many, I'm inclined to say we shouldn't touch BytecodeEmitter::countFinalSourceNotes.

This patch instead replaces SaveSharedScriptData's usage of script->numNotes() with a new argument, nsrcnotes. We have that value available at all callsites, so this is easy to do. It also very slightly reduces overhead, as script->numNotes() has to iterate over the source notes to find their count.

Comment 10

[Brian Hackett [Laid off!]]

Comment on attachment 781331 [details] [diff] [review]
part 1: remove functions from header files that are only used in jsscript.cpp

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

::: js/src/jsscript.cpp
@@ +386,5 @@
>  }
>  
> +/*
> + * Forward declaration for use in js::XDRScript
> + */

I don't think this comment is necessary.

Comment 11

[Till Schneidereit [:till]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5c0924e99992
https://hg.mozilla.org/integration/mozilla-inbound/rev/91bc683b2f45

@Matthew, thanks again for the fantastic report. Without the great testcase, it would've been very hard to track this down. And it looks like it might be the cause of other crashes, too.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5c0924e99992
https://hg.mozilla.org/mozilla-central/rev/91bc683b2f45

Comment 13

[Mike Kaply [:mkaply]]

We have an enterprise customer that is supposedly hitting this bug on the Firefox ESR

Is there a reason this bug was put into mozilla 25 and not backported at the time?

I believe that 23 was aurora at that point.

Comment 14

[Benjamin Smedberg]

This landed on Nightly as a normal bug and because it was not a top crash nor a security bug it just rode the trains as with any other bug.

Comment 15

[Lukas Blakk [:lsblakk] use ?needinfo]

(In reply to Mike Kaply (:mkaply) from comment #13)
> We have an enterprise customer that is supposedly hitting this bug on the
> Firefox ESR
> 
> Is there a reason this bug was put into mozilla 25 and not backported at the
> time?
> 
> I believe that 23 was aurora at that point.

If that customer can speak to reproducibility, frequency, and number of users affected we might consider this for an exception to the landing criteria but otherwise this will just ship in the next ESR.