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

RESOLVED FIXED in Firefox 28

Status

()

Core
JavaScript Engine
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jonco, Assigned: jonco)

Tracking

unspecified
mozilla28
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox28 fixed)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
Bug 942031 is looking mighty suspicious.
(Assignee)

Updated

4 years ago
Blocks: 942031
(Assignee)

Comment 2

4 years ago
Created attachment 8340377 [details] [diff] [review]
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.
Assignee: nobody → jcoppeard
Attachment #8340377 - Flags: review?(mh+mozilla)
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.
Attachment #8340377 - Flags: review?(mh+mozilla) → review-
(Assignee)

Updated

4 years ago
Depends on: 946732
(Assignee)

Updated

4 years ago
No longer depends on: 946732
(Assignee)

Comment 4

4 years ago
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.

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.
Attachment #8340377 - Attachment is obsolete: true
Attachment #8343171 - Flags: review?(mh+mozilla)
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.
Attachment #8343171 - Flags: review?(mh+mozilla) → review+
(Assignee)

Comment 6

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c0feb0c132a
https://hg.mozilla.org/mozilla-central/rev/0c0feb0c132a
https://hg.mozilla.org/mozilla-central/rev/8b341b232aa2
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
status-firefox28: --- → fixed
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.