391851 - Crash [@ JS_ResolveStandardClass] during shutdown after watch and delete

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

To reproduce, run ./js foo.js, where foo.js is:

this.watch('w', function(){});
delete w;

If you paste it into a shell instead of using a file, you'll have to add a
  quit();
to make it crash.


Exception:  EXC_BAD_ACCESS (0x0001)
Codes:      KERN_INVALID_ADDRESS (0x0001) at 0xdadadae2

Thread 0 Crashed:
0   js 	0x00014404 JS_ResolveStandardClass + 77 (jsapi.c:1450)
1   js 	0x0000765f global_resolve + 38 (js.c:2755)
2   js 	0x0008289f js_LookupPropertyWithFlags + 841 (jsobj.c:3282)
3   js 	0x00082554 js_LookupProperty + 53 (jsobj.c:3187)
4   js 	0x0002d904 DropWatchPointAndUnlock + 204 (jsdbgapi.c:367)
5   js 	0x0002edfe JS_ClearAllWatchPoints + 75 (jsdbgapi.c:847)
6   js 	0x00024a5c js_DestroyContext + 285 (jscntxt.c:387)
7   js 	0x00013abb JS_DestroyContext + 25 (jsapi.c:994)
8   js 	0x00007be9 main + 645 (js.c:3258)
9   js 	0x00001c86 _start + 216
10  js 	0x00001bad start + 41

Comment 1

[Brendan Eich [:brendan]]

I can't reproduce this on my MBP. Any hints? Can you give gdb-level detail of the local state in the crashing frame?

/be

Comment 2

[Jesse Ruderman]

Works fine after updating (thanks to the change in bug 386265?), but I still have a crashing build around.  Dunno if it's worth going after this crash now that it can't be reproduced, at least with the testcase here.  I'll attach a gdb session log.

Comment 3

[Jesse Ruderman]

Attachment: gdb session

Comment 4

[Igor Bukanov]

(In reply to comment #3)
> Created an attachment (id=276352) [details]
> gdb session
> 

Thanks for the trace, it made things clear. 

The bug still present and caused by the fact that js_DestroyContext calls JS_ClearAllWatchPoints after it calls js_FinishCommonAtoms. js_FinishCommonAtoms clears the common atoms and sets their storage storage to 0xdadadada in the debug build. Then JS_ClearAllWatchPoints accesses the atoms storage via js_LookupPropertyWithFlags that triggers ResolveStandardClass. 

The changes for bug 386265 just made the bug hidden as now ATOM_TO_STRING(atom) justs clear 3 lowest bit in the atom instead of dereferencing it. In the debug build it means that "idstr == ATOM_TO_STRING(atom)" is no longer true and crash is avoided. In the optimized build when the original atom is still stored in rt->atomState.typeAtoms[JSTYPE_VOID]; that check succeeds when id is "undefined", but even then this is "harmless" as it would just define a property in an object and root the atom again.

The fix should be simple: js_DestroyContext should call JS_ClearAllWatchPoints very early when destructing the last context.

Comment 5

[Igor Bukanov]

Attachment: fix v1

The fix just exits early in JS_ResolveStandardClass when the runtime is landing.

I decided not to change the order of JS_ClearAllWatchPoints and js_FinishCommonAtoms calls in js_DestroyContext as that does not fix the bug fully. An embedding may still call js_ResolveStandardClass directly or indirectly from, for example, GC hooks.

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 276368 [details] [diff] [review]
fix v1

Smart use of rt->state. May help in the future for similar bugs.

/be

Comment 7

[Igor Bukanov]

I checked in the patch from comment 5 to the trunk:

http://webtools.mozilla.org/registry/who.cgi?email=igor%25mir2.org&d=PhoenixTinderbox|HEAD|/cvsroot|1186956601|1186956720

Comment 8

[Igor Bukanov]

(In reply to comment #7)
> I checked in the patch from comment 5 to the trunk:
> 
> http://webtools.mozilla.org/registry/who.cgi?email=igor%25mir2.org&d=PhoenixTinderbox|HEAD|/cvsroot|1186956601|1186956720
> 

The proper bonsai link is:

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1186956601&maxdate=1186956720&who=igor%mir2.org

Comment 9

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/extensions/regress-391851.js

Comment 10

[Bob Clary [:bc] (inactive)]

verified fixed 1.9.0 linux/mac*/windows.

Comment 11

[Bob Clary [:bc] (inactive)]

when this bug is opened, the test should be checked in.