Closed Bug 897507 Opened 8 years ago Closed 8 years ago

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


(Core :: JavaScript Engine, defect)

21 Branch
Not set



Tracking Status
firefox-esr24 - ---


(Reporter: mmaxwell, Assigned: till)



(Keywords: crash, regression, testcase)

Crash Data


(4 files)

Attached file
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.
Attachment #780433 - Attachment mime type: application/octet-stream → application/zip
Regression range:
Severity: normal → critical
Crash Signature: [@ js::frontend::IsIdentifier(JSLinearString*) ]
Ever confirmed: true
Assignee: nobody → general
Component: Untriaged → JavaScript Engine
Keywords: reproducible
Product: Firefox → Core
Hardware: x86_64 → All
Version: 22 Branch → 21 Branch
Regression window(m-i)
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20130213 Firefox/21.0 ID:20130213094627
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20130213 Firefox/21.0 ID:20130213101725

Regressed by:
985508c04c80	Till Schneidereit — Bug 679940 - Share bytecode, source notes and atoms of functions in a runtime wherever possible. r=bhackett
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.
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.
Oh, I see. Thanks for the explanation - that spares me from going on a wild goose chase for bugs related to the semicolons.
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.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.
Flags: needinfo?(bhackett1024)
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.
Attachment #781331 - Flags: review?(bhackett1024)
Assignee: general → till
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.
Attachment #781336 - Flags: review?(bhackett1024)
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.
Attachment #781331 - Flags: review?(bhackett1024) → review+
Attachment #781336 - Flags: review?(bhackett1024) → review+

@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.
Flags: needinfo?(bhackett1024)
OS: Windows 7 → All
Version: 21 Branch → 25 Branch
Version: 25 Branch → 21 Branch
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
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.
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.
(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.
You need to log in before you can comment on or make changes to this bug.