942928 - GenerationalGC: Browser opt builds fail with undefined reference to `JS::HeapCellRelocate(js::gc::Cell**)'

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jon Coppeard (:jonco)]

From https://tbpl.mozilla.org/php/getParsedLog.php?id=31046656&full=1&branch=try :

../../signalingtest/signaling_ecc/MediaStreamList.o: In function `js::GCMethods<JSObject*>::relocate(JSObject**)':
/builds/slave/try-l64-0000000000000000000000/build/obj-firefox/media/webrtc/signalingtest/signaling_ecc/../../../../dist/include/js/RootingAPI.h:642: undefined reference to `JS::HeapCellRelocate(js::gc::Cell**)'
collect2: error: ld returned 1 exit status
make[5]: *** [mediapipeline_unittest] Error 1

It's been a while since I tried an opt build so I don't know when this happened.

Comment 1

[Jon Coppeard (:jonco)]

Bug 942031 is looking mighty suspicious.

Comment 2

[Jon Coppeard (:jonco)]

Attachment: fix-ggc-browser-builds

The test harness above (and probably more) are failing to build with generational garbage collection because they're no longer linked against the JS engine.  With GGC, any code that manipulates JS values on the heap needs to be linked against the JS engine, even if it doesn't all any JS APIs directly.

This patch just restores test code to link the JS engine as it was before bug 942031.

Comment 3

[Mike Hommey [:glandium]]

Comment on attachment 8340377 [details] [diff] [review]
fix-ggc-browser-builds

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

First things first, when you have a linkage failure on something, don't assume the solution for it would work everywhere. Because the js engine, on linux and mac, is a static library. The CPP unit tests link against libxul, which contains the js engine. If you link a CPP unit test to both, you get a program that, if it calls the js engine, will call the one statically linked to it. And libxul would use its own copy. Imagine the kind of fun problems that can follow from there. We've been lucky that it hasn't been a problem before bug 942031.

Now, with that being said, your build error is on a special case of unit tests. Touching something in gecko with those tests around is like playing catchball with a hand grenade without a pin, and trying to be careful that the lever doesn't go off. Because they both dynamically link to libxul as well as statically link to a bunch of things libxul contains. Which things, in all likeliness, are what require those GGC symbols. So that's really where you want the js engine linked. But that's just going to delay the timebomb.

Comment 4

[Jon Coppeard (:jonco)]

Attachment: fix-ggc-link-error-v3

I don't understand the build system that well so I may be wrong, but the problem here appears to be that these unit tests are not linked against libxul and so require linking against JS explicitly.

The MediaStreamList class is derived from nsWrapperCache, which has a JS::Heap<JSObject*> memeber, which is where the dependency is coming from.

Here's a patch to add the JS to the libraries linked for the webrtc/signalling tests, which fixes the build errors.

Comment 5

[Mike Hommey [:glandium]]

Comment on attachment 8343171 [details] [diff] [review]
fix-ggc-link-error-v3

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

(In reply to Jon Coppeard (:jonco) from comment #4)
> Created attachment 8343171 [details] [diff] [review]
> fix-ggc-link-error-v3
> 
> I don't understand the build system that well so I may be wrong, but the
> problem here appears to be that these unit tests are not linked against
> libxul and so require linking against JS explicitly.

They *are* linked against libxul, but they are also statically linked to plenty of the webrtc guts. *That* is what requires linking js explicitly, and that's also what is the timebomb. Linking without libxul actually failed last time i tried. If something actually ends up calling into it, it's going to blow up. We're lucky that it works.

Comment 6

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0c0feb0c132a

Comment 7

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/0c0feb0c132a
https://hg.mozilla.org/mozilla-central/rev/8b341b232aa2