1235598 - Convert Gecko over to the C++ Tracing API

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Terrence Cole [:terrence]]

Attachment: use_TraceEdge_from_embeddng-sm-v0.diff

I only had to beef up support on SpiderMonkey's side a little bit. There were two main things I touched.

First, now that adding a new trace variant is trivial, I added JS::TraceNullableEdge. This made a fairly large number of places simpler, so I'm confident that it's a clear net win.

Secondly, I ensured that JS::Trace* are only instantiated for externally exposed types. This is more important than it seems: if we accidentally expose a new object sub-class without adding manual barrier overrides to implement post-barriers, we have a latent sec-crit bug. With this change, we will now fail to build -- a link failure, but better than nothing -- if that occurs. It's a bit indirect, but plugs an important gap in our type coverage that I've been worried about for awhile now.

Comment 1

[Terrence Cole [:terrence]]

Attachment: use_TraceEdge_from_embeddng-gk-v0.diff

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8702623 [details] [diff] [review]
use_TraceEdge_from_embeddng-gk-v0.diff

 IdToObjectMap::trace(JSTracer* trc)
 {
-    for (Table::Range r(table_.all()); !r.empty(); r.popFront()) {
-        DebugOnly<JSObject*> prior = r.front().value().get();
-        JS_CallObjectTracer(trc, &r.front().value(), "ipc-object");
-    }
+    for (Table::Range r(table_.all()); !r.empty(); r.popFront())
+        JS::TraceEdge(trc, &r.front().value(), "ipc-object");

I wouldn't drop {}, but I guess this file is using some odd coding style.

Comment 3

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8702622 [details] [diff] [review]
use_TraceEdge_from_embeddng-sm-v0.diff

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

This is good stuff.

Comment 4

[Terrence Cole [:terrence]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=75f433069cf2

Comment 5

[Terrence Cole [:terrence]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fba9dbd3f6bc

Comment 6

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/55db488f9dfd078a927cb706001d65ef38da3219
Bug 1235598 - Part 1: Add better SpiderMonkey API support for tracing in C++; r=sfink

https://hg.mozilla.org/integration/mozilla-inbound/rev/cf35918cb3baff6e2c67209fa3cf0b0d03771073
Bug 1235598 - Part 2: Use TraceEdge exclusively in Gecko; r=smaug

Comment 7

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7da07459ce20c444c070ba8061c687c77bbc5400
Backout Bug 1235598 Part 2 because of merge bustage on a CLOSED TREE

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed it all out because of build bustage: https://hg.mozilla.org/integration/mozilla-inbound/rev/c1b8d8e11880

https://treeherder.mozilla.org/logviewer.html#?job_id=19134343&repo=mozilla-inbound

Comment 9

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/80cd10a8b3d7a2e249a7632dd442306e7f0b8890
Bug 1235598 - Part 1: Add better SpiderMonkey API support for tracing in C++; r=sfink

https://hg.mozilla.org/integration/mozilla-inbound/rev/3bcd3c276785b20eaa1b3ffac83149ae1d3a8b18
Bug 1235598 - Part 2: Use TraceEdge exclusively in Gecko; r=smaug

Comment 10

[Terrence Cole [:terrence]]

This was merge bustage: I fixed the new tracing that Olli landed yesterday, but in the wrong patch.

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back these out for apparently turning linux reftests orange:

https://hg.mozilla.org/integration/mozilla-inbound/rev/4bbe608d70f0


Example failures:
https://treeherder.mozilla.org/logviewer.html#?job_id=19164298&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=19164299&repo=mozilla-inbound

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

You also seem to have upset about half of the b2g emulator reftests.

Comment 13

[Terrence Cole [:terrence]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=777e1dd17331

Comment 14

[Terrence Cole [:terrence]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=27afd7eecd5a

Comment 15

[Terrence Cole [:terrence]]

Well, there's basically no way that a tracing change could result in an image comparison failure (as opposed to a crash). I now have two try runs at [1] and [2], one before rebasing and one after, that don't show those errors. Or at least not in the same place. It's hard to tell what's going on with all the default orange in try runs recently.

1 - https://treeherder.mozilla.org/#/jobs?repo=try&revision=216179f9d9ba
2 - https://treeherder.mozilla.org/#/jobs?repo=try&revision=adf94c00e677

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Oh, I thought I had followed up in here. These failures turned out to be from the libmesa upgrade that was rolled out to ec2 instances. That upgrade was rolled back due to all of the fallout. Feel free to reland these whenever.

Comment 17

[Terrence Cole [:terrence]]

(In reply to Wes Kocher (:KWierso) (On vacation until Dec 28) from comment #16)
> Oh, I thought I had followed up in here. These failures turned out to be
> from the libmesa upgrade that was rolled out to ec2 instances. That upgrade
> was rolled back due to all of the fallout. Feel free to reland these
> whenever.

Great! Thought I was going crazy there for a bit.

Comment 18

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/25ed386771f30c922b402a8c5d25f3fa13b98ed6
Bug 1235598 - Part 1: Add better SpiderMonkey API support for tracing in C++; r=sfink

https://hg.mozilla.org/integration/mozilla-inbound/rev/d27aac8b654a3711c899107e36781a01c165e895
Bug 1235598 - Part 2: Use TraceEdge exclusively in Gecko; r=smaug

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/25ed386771f3
https://hg.mozilla.org/mozilla-central/rev/d27aac8b654a