490044 - TM: Add deep-bailing write barrier to global shape change code

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Graydon Hoare :graydon]

This is the "more complete" version of the fix presented in bug 488363, split off for independent tracking.

Comment 1

[Graydon Hoare :graydon]

Attachment: Refreshed copy of wip from other bug

This variant includes the proposed formatting and naming fixes in your last review, as well as the two additional shape-change cases I missed.

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 374518 [details] [diff] [review]
Refreshed copy of wip from other bug

Had to diff the patches, looks good.

/be

Comment 3

[Graydon Hoare :graydon]

First landing was http://hg.mozilla.org/tracemonkey/rev/d8c2060b0f9b .

This caused cross-the-board red tinderboxes: crashes during object-tracing. I guess I was a little too optimistic that it was a conservative little patch, and was on a laptop so didn't do a full browser test run. So never saw cycle collection run.

The culprit was the line added in js_TraceObject. I backed that line out; I wasn't entirely confident in it in the first place, since I don't really know the invariants of the tracing phase of CC. Anyway, current tip is http://hg.mozilla.org/tracemonkey/rev/07bdf85b247a , which appears to be settling down to green, though there's the one funny winnt orange again, and 2 taloses that don't seem to want to cycle (the taloses that *did* cycle are fine). 

It's nearly 2am now -- I was hoping this would wind down sooner -- so I'm off to bed now. If it's not resolved back to green by tomorrow morning I'll back out fully.

Assuming it sticks, porting to other branches should take both changes.

Comment 4

[Brendan Eich [:brendan]]

Sorry, I should have caught that -- we have other ways of leaving trace if GC is activated. What was the crash stack?

/be

Comment 5

[Graydon Hoare :graydon]

(In reply to comment #4)
> Sorry, I should have caught that -- we have other ways of leaving trace if GC
> is activated. What was the crash stack?

A representative would be, say, http://tinderbox.mozilla.org/showlog.cgi?log=TraceMonkey/1240637337.1240638111.3015.gz

They all look like this. Summary of stack:

Crash reason:  EXC_BAD_ACCESS / KERN_PROTECTION_FAILURE
Crash address: 0x251467

Thread 0 (crashed)
 0  js_TraceObject
 1  JS_CallTracer
 2  fun_trace
 3  js_TraceObject
 4  JS_CallTracer
 5  js_TraceScopeProperty
 6  js_TraceObject
 7  JS_CallTracer
 8  xpc_TraceForValidWrapper()
 9  js_TraceObject
10  JS_CallTracer
11  js_TraceObject
12  JS_CallTracer
13  js_CallValueTracerIfGCThing
14  gc_root_traversal
15  JS_DHashTableEnumerate
16  js_TraceRuntime
17  js_GC
18  JS_GC
19  nsXREDirProvider::DoShutdown()
20  ScopedXPCOMStartup::RegisterProfileService()
21  XRE_main
22  main

Pretty straightforward. I think the owning object was probably dead? *shrug* as you say, I think the odds of us being on trace during GC are zero anyway. It's caught elsewhere.

Comment 6

[Brendan Eich [:brendan]]

The "trace" (formerly "mark") phase of the GC is scanning live things, so the owning object had better not be dead. Any bad address info? With the inlining of js_LeaveTraceIfGlobalObject perhaps the crash was in that function.

We should rename the GC "trace" phase, if we can (recent-ish API).

/be

Comment 7

[Graydon Hoare :graydon]

Asked build to kick it again since nothing happened overnight; all green now, looks like it was just a lot of transient noise. I've filed [orange] bugs for all of them.

Comment 8

[ticoi20]

Attachment: 490044-possible exploit.

Comment 9

[Graydon Hoare :graydon]

(In reply to comment #8)
> Created an attachment (id=374819) [details]
> 490044

Um, what's this attachment? It's a PE binary claiming to be firefox.exe, but it sure doesn't look like that on the inside. It's got a few bits that look firefoxy (strings and imports and whatnot) but it's only 80kb. I'm not terribly interested in running it.

Comment 10

[Brendan Eich [:brendan]]

I removed user [e-mail].

/be

Comment 11

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/d8c2060b0f9b

(this bug is still fixed, right?)

Comment 12

[Graydon Hoare :graydon]

(In reply to comment #11)
> http://hg.mozilla.org/mozilla-central/rev/d8c2060b0f9b
> 
> (this bug is still fixed, right?)

Yes, so long as you got the followup commit: 

http://hg.mozilla.org/tracemonkey/rev/07bdf85b247a

It appears you did, http://hg.mozilla.org/mozilla-central/rev/07bdf85b247a

(There was some wobbling with random oranges after *that* landed too, but nothing evidently related to this bug. And then some fellow appears to have decided to attach what looks like malware to the bug; I have no idea why. But otherwise I believe it's ok.)

Comment 13

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/a4b589a68a8e