1028863 - Assertion failure: hasTwoByteChars(), at vm/String.h:1460 or Crash [@ js::StringBuffer::append] with saveStack

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase asserts on mozilla-central revision 366b5c0c02d3 (run with --fuzzing-safe):


function writeTestCaseResult( expect, actual, string ) {}
schedulegc(10);
if (saveStack() == 3) done = true;

Comment 1

[Christian Holler (:decoder)]

Attachment: [crash-signature] Machine-readable crash signature

Comment 2

[Christian Holler (:decoder)]

Marked as fuzzblocker because this triggers quite easily.

Comment 3

[Christian Holler (:decoder)]

Attachment: [crash-signature] Machine-readable crash signature

Comment 4

[Christian Holler (:decoder)]

Attachment: [crash-signature] Machine-readable crash signature

Comment 5

[Christian Holler (:decoder)]

JSBugMon: Bisection requested, failed due to error (try manually).

Comment 6

[Jon Coppeard (:jonco)]

SavedStacks::insertFrames() does this:

    SavedFrame::Lookup lookup(location.source,
                              location.line,
                              location.column,
                              callee ? callee->displayAtom() : nullptr,
                              parentFrame,
                              thisFrame.compartment()->principals);

    frame.set(getOrCreateSavedFrame(cx, lookup));


Lookup contains unrooted GC things and getOrCreateSavedFrame() creates an new JSObject before using its contents, so this should really be a rooting hazard.  However, lookup is passed as a reference so at a guess the analysis assumes it's rooted in createFrameFromLookup() where it's is used after the possibility of GC.

I checked and this is one of our unsafe refs:

Function '_ZN2js11SavedStacks12insertFramesEP9JSContextRNS_15ScriptFrameIterEN2JS13MutableHandleIPNS_10SavedFrameEEE|uint8 js::SavedStacks::insertFrames(JSContext*, js::ScriptFrameIter*, class JS::MutableHandle<js::SavedFrame*>)' takes unsafe address of unrooted 'lookup' at js/src/vm/SavedStacks.cpp:464
    js/src/vm/SavedStacks.cpp:464: Call(35,36, __temp_14 := this*.getOrCreateSavedFrame(cx*,lookup))

It would be great if we could make this be reported as a hazard somehow.  It also means we should audit the unsafe refs list again.

Comment 7

[Jon Coppeard (:jonco)]

Attachment: bug1028863-root-lookup

Add an autorooter for SavedFrame::Lookup and use this to root Lookup around object creation.

I also added typedefs for HandleSavedFrame etc.

Comment 8

[Terrence Cole [:terrence]]

Comment on attachment 8444427 [details] [diff] [review]
bug1028863-root-lookup

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

Gah! When reviewing, I walked through the places where a Lookup is on the stack call by call with mrgiggles and figured the analysis would catch anything I missed. Just goes to show how impossible this stuff is to get right even with all the helpful tools we've built. I'd consider this bug as solid positive evidence that we need to take the time to push the unsafe refs to 0 too: I'll add it to our list for triaging.

Comment 9

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

> JSBugMon: Bisection requested, failed due to error (try manually).

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/466812b83481
user:        Nick Fitzgerald
date:        Tue Jun 17 16:32:00 2014 -0400
summary:     Bug 1004110 - Memoize PCToLineNumber lookups in SavedStacks. r=terrence

Guessing this might be due to bug 1004110...

Comment 10

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6dbb4388563b

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/6dbb4388563b