Last Comment Bug 783315 - Intermittent crash in jsreftest.html?test=js1_8_1/regress/regress-452498-117.js [@ js::gc::MarkInternal<js::PropertyName>]
: Intermittent crash in jsreftest.html?test=js1_8_1/regress/regress-452498-117....
Status: RESOLVED FIXED
: crash, intermittent-failure
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86_64 Mac OS X
-- critical (vote)
: mozilla17
Assigned To: Luke Wagner [:luke]
:
: Jason Orendorff [:jorendorff]
Mentors:
: 783278 783543 (view as bug list)
Depends on: 783278
Blocks: 438871
  Show dependency treegraph
 
Reported: 2012-08-16 10:02 PDT by Ed Morley [:emorley]
Modified: 2012-11-25 19:31 PST (History)
6 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix (11.74 KB, patch)
2012-08-16 13:27 PDT, Luke Wagner [:luke]
wmccloskey: review+
Details | Diff | Splinter Review

Description User image Ed Morley [:emorley] 2012-08-16 10:02:30 PDT
+++ This bug was initially created as a clone of Bug #783278 +++

Rev4 MacOSX Lion 10.7 mozilla-inbound debug test jsreftest on 2012-08-16 09:29:28 PDT for push 4392d5928cf1

slave: talos-r4-lion-020

https://tbpl.mozilla.org/php/getParsedLog.php?id=14440085&tree=Mozilla-Inbound

{
REFTEST TEST-START | file:///Users/cltbld/talos-slave/test/build/jsreftest/tests/jsreftest.html?test=js1_8_1/regress/regress-452498-117.js | 2925 / 3198 (91%)
++DOMWINDOW == 80 (0x146e7e320) [serial = 5488] [outer = 0x1111a1890]
BUGNUMBER: 452498
STATUS: TM: upvar2 regression tests
TEST-UNEXPECTED-FAIL | file:///Users/cltbld/talos-slave/test/build/jsreftest/tests/jsreftest.html?test=js1_8_1/regress/regress-452498-117.js | Exited with code 1 during test run
INFO | automation.py | Application ran for: 0:12:56.183104
INFO | automation.py | Reading PID log: /var/folders/qd/srwd5f710sj0fcl9z464lkj00000gn/T/tmphS5nmlpidlog
Downloading symbols from: http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-inbound-macosx64-debug/1345132906/firefox-17.0a1.en-US.mac64.crashreporter-symbols.zip
PROCESS-CRASH | file:///Users/cltbld/talos-slave/test/build/jsreftest/tests/jsreftest.html?test=js1_8_1/regress/regress-452498-117.js | application crashed (minidump found)
Crash dump filename: /var/folders/qd/srwd5f710sj0fcl9z464lkj00000gn/T/tmp3cVUcc/minidumps/0768A338-BEF1-43F6-B029-646A2C2F1CD6.dmp
Operating system: Mac OS X
                  10.7.2 11C74
CPU: amd64
     family 6 model 23 stepping 10
     2 CPUs

Crash reason:  EXC_BAD_ACCESS / 0x0000000d
Crash address: 0x0

Thread 0 (crashed)
 0  XUL!js::gc::MarkInternal<js::PropertyName> [Heap.h : 1010 + 0x0]
    rbx = 0x05c15268   r12 = 0x031690b1   r13 = 0xcdcdc000   r14 = 0xcdcdcdc8
    r15 = 0x5fbf7240   rip = 0x02e326cb   rsp = 0x5fbf7200   rbp = 0x5fbf7230
    Found by: given as instruction pointer in context
 1  XUL!JSScript::markChildren [jsscript.cpp : 232 + 0xe]
    rip = 0x02d12b3a   rsp = 0x5fbf7240
    Found by: stack scanning
 2  XUL!js::gc::PushArenaTyped<JSScript> [Marking.cpp : 773 + 0xa]
    rip = 0x02e313a2   rsp = 0x5fbf7280
    Found by: stack scanning
 3  XUL!js::gc::PushArena [Marking.cpp : 929 + 0xa]
    rip = 0x02e2d17d   rsp = 0x5fbf72f0
    Found by: stack scanning
}
Comment 1 User image Treeherder Robot 2012-08-16 10:48:37 PDT
philor
https://tbpl.mozilla.org/php/getParsedLog.php?id=14440940&tree=Mozilla-Inbound
Rev4 MacOSX Lion 10.7 mozilla-inbound debug test jsreftest on 2012-08-16 10:21:50
slave: talos-r4-lion-009

TEST-UNEXPECTED-FAIL | file:///Users/cltbld/talos-slave/test/build/jsreftest/tests/jsreftest.html?test=js1_8_1/regress/regress-452498-117.js | Exited with code 1 during test run
PROCESS-CRASH | file:///Users/cltbld/talos-slave/test/build/jsreftest/tests/jsreftest.html?test=js1_8_1/regress/regress-452498-117.js | application crashed (minidump found)
Thread 0 (crashed)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | missing output line for total leaks!
Comment 2 User image Andrew McCreight [:mccr8] 2012-08-16 10:50:44 PDT
If I'm reading the code correctly, I think Luke added this MarkInternal<PropertyName> thing in bug 775323.
Comment 3 User image Luke Wagner [:luke] 2012-08-16 12:13:50 PDT
You're right.  I just reproduced in a debugger; I seem to have a JSScript where almost, but not all, of the fields have been nulled out.  I wonder if it is one of these degenerate scripts which doesn't call the normal JSScript init functions...
Comment 4 User image Luke Wagner [:luke] 2012-08-16 13:27:47 PDT
Created attachment 652542 [details] [diff] [review]
fix

This is a fun corner case: if there is an error after generating bindings but before a script is fully emitted, then Bindings can be left pointing into the LifoAlloc'd storage.  This would be fine if the script wasn't reachable but apparently it was being found in this test via conservative stack scanning.

With this patch, I can't reproduce the crash.
Comment 5 User image Andrew McCreight [:mccr8] 2012-08-16 14:31:39 PDT
Bug 783278 has what is probably the same crash, if you haven't checked that out already.
Comment 6 User image Luke Wagner [:luke] 2012-08-16 15:29:43 PDT
Thanks, yes, looks like the same thing.
Comment 7 User image Bill McCloskey (:billm) 2012-08-16 15:52:16 PDT
Comment on attachment 652542 [details] [diff] [review]
fix

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

::: js/src/jsscript.cpp
@@ +75,5 @@
>          return false;
>      }
>  
> +    JS_ASSERT(!(size_t(bindingArray) & TEMPORARY_STORAGE_BIT));
> +    bindingArrayAndFlag_ = size_t(bindingArray) | TEMPORARY_STORAGE_BIT;

size_t -> uintptr_t

@@ +139,4 @@
>  {
> +    JS_ASSERT(bindingArrayUsingTemporaryStorage());
> +    PodCopy(newBindingArray, bindingArray(), count());
> +    bindingArrayAndFlag_ = size_t(newBindingArray);

uintptr_t. Also, you should probably check the low bit here if you check it above. Maybe have a private setter for this field?

@@ +238,2 @@
>          PropertyName *name = b->name();
>          MarkStringUnbarriered(trc, &name, "bindingArray");

What keeps this stuff alive during parsing? gcKeepAtoms? If so, please comment about it.

::: js/src/jsscript.h
@@ +129,5 @@
>      friend class BindingIter;
>      friend class AliasedFormalIter;
>  
>      HeapPtr<Shape> callObjShape_;
> +    size_t bindingArrayAndFlag_;

I think this should be a uintptr_t.

@@ +143,5 @@
> +     * error path, which is error-prone, the solution used it to not mark
> +     * bindingArray when it points to temporary storage. This is safe since
> +     * compilation already keeps atoms alive (hence no rooting is necessary).
> +     * The "temporary" status is tracked a bit stolen from the bindingArray
> +     * pointer.

This comment seems too low-level to me. I think something like this would be more helpful (unless this info is available elsewhere):

During parsing, bindings are allocated in a temporary LifoAlloc arena. After parsing, a JSScript object is created and the bindings are permanently transferred to it. On some error paths, the JSScript object may end up with bindings that still point to the (now dead) LIFO memory. To avoid tracing these bindings during GC, we keep track of whether the bindings are temporary or permanent in the low bit of bindingArrayAndFlag_.
Comment 8 User image Luke Wagner [:luke] 2012-08-16 17:37:16 PDT
(In reply to Bill McCloskey (:billm) from comment #7)
Thanks!

https://hg.mozilla.org/integration/mozilla-inbound/rev/c068429832ea
Comment 9 User image Phil Ringnalda (:philor) 2012-08-16 22:57:14 PDT
https://tbpl.mozilla.org/php/getParsedLog.php?id=14455738&tree=Ionmonkey (in js1_8_1/extensions/regress-437288-01.js)
Comment 10 User image Treeherder Robot 2012-08-17 00:51:42 PDT
cjones
https://tbpl.mozilla.org/php/getParsedLog.php?id=14454447&tree=Try
Rev4 MacOSX Lion 10.7 try debug test jsreftest on 2012-08-16 19:13:55
slave: talos-r4-lion-033

TEST-UNEXPECTED-FAIL | file:///Users/cltbld/talos-slave/test/build/jsreftest/tests/jsreftest.html?test=js1_8_1/regress/regress-452498-117.js | Exited with code 1 during test run
PROCESS-CRASH | file:///Users/cltbld/talos-slave/test/build/jsreftest/tests/jsreftest.html?test=js1_8_1/regress/regress-452498-117.js | application crashed (minidump found)
Thread 0 (crashed)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | missing output line for total leaks!
Comment 11 User image Ed Morley [:emorley] 2012-08-17 05:28:58 PDT
https://hg.mozilla.org/mozilla-central/rev/c068429832ea
Comment 12 User image Luke Wagner [:luke] 2012-08-17 07:53:34 PDT
*** Bug 783278 has been marked as a duplicate of this bug. ***
Comment 13 User image Luke Wagner [:luke] 2012-08-17 07:54:16 PDT
*** Bug 783543 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.