1017650 - Mark objects black at API boundaries

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Terrence Cole [:terrence]]

Currently we have a hodge-podge of ExposeToActiveJS, GetObjectPreserveColor, and other wackiness scattered about that probably more or less works most of the time. We can do much better here.

Comment 1

[Bobby Holley (:bholley)]

Which boundaries exactly? It always seemed to me that we should do gray unmarking when things move from JS::Heap to JS::Rooted/JS::Handle. Would that make sense?

Comment 2

[Terrence Cole [:terrence]]

(In reply to Bobby Holley (:bholley) from comment #1)
> Which boundaries exactly? It always seemed to me that we should do gray
> unmarking when things move from JS::Heap to JS::Rooted/JS::Handle. Would
> that make sense?

I was going to spend a bit thinking about it before opining and probably needinfo you and Bill asking if I was on the right track. Thank you for saving me the trouble! :-)

Comment 3

[Bobby Holley (:bholley)]

Here's a conversation we had about this over email 18 months ago:

> ----- Original Message -----
> > From: "Bobby Holley" <[e-mail]>
> > My thinking is to use HandleObject API to enforce non-grayness.
> > Specifically, we'd have the invariant that all HandleObjects were
> > non-gray,
> > since in the common case any handles we're passing as arguments will
> > probably eventually enter JSAPI. If we needed to, we could introduce
> > a
> > GrayableHandleObject with automatic coercion to HandleObject via a
> > call to
> > xpc_UnmarkGrayObject.

On Thu, Dec 27, 2012 at 1:39 PM, Bill McCloskey <[e-mail]> wrote:
> The new rooting API isn't really ready for this right now. Except for callbacks, we barely use handles at all in JSAPI. I'm guessing we're at least a few months away from starting to add handles to JSAPI. We need to get them working inside the JS engine first.
>
> However, it seems like a great idea for the future. Whatever rooters we expose in JSAPI could call xpc_UnmarkGray automatically. That might even allow us to eliminate all the other calls to xpc_UnmarkGray (except for ones during the forget skippable phase of cycle collection I guess).
>
> -Bill

Comment 4

[Terrence Cole [:terrence]]

Thanks Bobby! The Rooted/Handle infrastructure is now in an excellent position to do this. I'll try to whip something up soon.

Comment 5

[Terrence Cole [:terrence]]

Attachment: auto_unmark_gray-v0.diff

https://tbpl.mozilla.org/?tree=Try&rev=b160cb165cf2

This does ExposeToActiveJS whenever we move a TenuredHeap or Heap into a Rooted and adds assertions to Handle that we are not marked gray. The attached patch builds locally and renders cnn.com without triggering any of the new asserts in Handle. This may be because ExposeToActiveJS is still called everywhere in the browser: removing all of those is the next step.

This most challenging thing here (after understanding how gray marking works) was changing the include dependencies between Value.h, RootingAPI.h, GCAPI.h, and Heap.h so that we can actually call ExposeToActiveJS from RootingAPI.h.

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

It looks like this calls ExposeToActiveJS only when the thing is marked gray. That's not sufficient though. We also need to call it during incremental GC:

http://mxr.mozilla.org/mozilla-central/source/js/public/GCAPI.h#490

During incremental, we buffer up stuff that we plan to mark gray later on. Gray marking happens in one atomic step right before sweeping. If somebody calls ExposeToActiveJS before that, we need to specifically mark it black so that it doesn't get marked gray.

Comment 7

[Terrence Cole [:terrence]]

Comment on attachment 8433407 [details] [diff] [review]
auto_unmark_gray-v0.diff

Thanks, Bill, good catch!

Comment 8

[Terrence Cole [:terrence]]

Attachment: rearrange_gcapi_include_order-v0.diff

As a first step we need to make ExposeToActiveJS callable from RootingAPI.h. Currently we have (foo <- bar means foo.h includes bar.h):

GCAPI <- RootingAPI  (So ObjectPtr can use Heap<T>)
      <- Value       (So GCAPI can implement ExposeValueToActiveJS)

This patch simply inverts those two includes and moves the relevant code to the other file. This allows RootingAPI to include GCAPI and Value and gain access to all of the Expose*ToActiveJS it needs.

This was green on try, so getting it out of the way first.

Comment 9

[Terrence Cole [:terrence]]

Attachment: auto_expose_to_active_js_from_heap-v0.diff

https://tbpl.mozilla.org/?tree=Try&rev=eb883a177e03

Actually do the ExposeToActiveJS. This doesn't add the assertions because currently does absolutely nothing. The browser never goes straight from Heap<T> -> Rooted<T>, it always has an intermediate accessor that returns the raw pointer. I think the right solution here is going to be to make all of the intermediate accessors return a const ref to the Heap<T>. We should be able to hunt these down by looking for all the ExposeToActiveJS, but it's going to be a bit of work.

Comment 11

[Terrence Cole [:terrence]]

I forgot to add a null test to ExposeToActiveJS when I added one to isGrayGCThing.

https://tbpl.mozilla.org/?tree=Try&rev=bac3026937be

Comment 12

[Terrence Cole [:terrence]]

Attachment: auto_expose_to_active_js_from_heap-v2.diff

https://tbpl.mozilla.org/?tree=Try&rev=8e61d0f6712d

This patch trips up when console.log is passed a string that happens to be a PermanentAtom because js::IncrementalReferenceBarrier was not able to handle those. The new patch adds an explicit check for this case.

With this change, I'm pretty sure we're finally catching all the new edge cases.

Comment 13

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8438575 [details] [diff] [review]
auto_expose_to_active_js_from_heap-v2.diff

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

This looks nice. I'd like to wait on reviewing until the PreserveColor stuff is ready though.

::: js/public/HeapAPI.h
@@ +264,5 @@
>  
>  static MOZ_ALWAYS_INLINE bool
>  GCThingIsMarkedGray(void *thing)
>  {
> +    if (js::gc::IsNullTaggedPointer(thing))

I think the only time you need to worry about values in [1, 32) is for TaggedProto. I'd rather just have a special check there if possible.

Comment 14

[Terrence Cole [:terrence]]

Attachment: remove_unused_proto_arg-v0.diff

(In reply to Bill McCloskey (:billm) from comment #13)
> I think the only time you need to worry about values in [1, 32) is for
> TaggedProto. I'd rather just have a special check there if possible.

Easier said than done, sadly. Here is the first half.

Comment 15

[Eric Faust [:efaust]]

Comment on attachment 8440176 [details] [diff] [review]
remove_unused_proto_arg-v0.diff

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

Nice. Thanks for doing this. It moved to ProxyOptions (or maybe WrapperOptions?) a while ago. r=me

Comment 16

[Terrence Cole [:terrence]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1fde1342a1cb
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/23934a59230e

Comment 17

[Andrew McCreight [:mccr8]]

Please be sure to get a DOM reviewer or whatever for Gecko changes.  Olli or I will probably work.

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1fde1342a1cb
https://hg.mozilla.org/mozilla-central/rev/23934a59230e

Comment 19

[Jon Coppeard (:jonco)]

This work was finished up in bug 1297558.