leper Reporter
	Description • 8 years ago

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:43.0) Gecko/20100101 Firefox/43.0 SeaMonkey/2.40
Build ID: 20160807101136

Steps to reproduce:

When compiling the attached test program (based on the helloworld example from https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/How_to_embed_the_JavaScript_engine with a few fixes for trunk) which include a "JS::Rooted<JS::PropertyDescriptor> desc(cx);" linking fails as JS::PropertyDescriptor::trace is undefined.

libmozjs-52a1 was built according to https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Build_Documentation.

g++ -std=c++11 -I<obj_dir>/dist/include -L<obj_dir>/dist/bin helloworld.cpp -o helloworld -lmozjs-52a1 -lz -lpthread -ldl -Wl,--whole-archive <obj_dir>/dist/sdk/lib/libmozglue.a -Wl,--no-whole-archive



Expected results:

Linking should not fail, making JS::PropertyDescriptor part of JS_PUBLIC_API fixes this. Note that this issue was mentioned in Bug 1209684.

	Pulsebot
	Comment 2 • 8 years ago

Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1c5df3c47a4
Mark JS::PropertyDescriptor as JS_PUBLIC_API to fix linking. r=luke

	Pulsebot
	Comment 3 • 8 years ago

Pushed by shu@rfrn.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e53345bea4f
Followup: wrap forward decls of PropertyDescriptor with JS_PUBLIC_API. (r=me)

	Daniel Holbert [:dholbert]
	Comment 4 • 8 years ago

At shu's request, I backed out the main patch here:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c97b9c7000a
...for introducing build bustage like this:
https://treeherder.mozilla.org/logviewer.html#?job_id=38846773&repo=mozilla-inbound

And I backed out shu's followup (which was intended to fix that build bustage):
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f3b77d736fa
...because it introduced separate bustage like this:
https://treeherder.mozilla.org/logviewer.html#?job_id=38853760&repo=mozilla-inbound

You'll want to use Try Server to get an updated fix tested & verified-to-not-cause-bustage here.

	leper Reporter
	Comment 5 • 8 years ago

Updated the patch, it would be great if someone could push this to the try server.

The additional includes are needed because of https://gcc.gnu.org/bugzilla/show_bug.cgi?id=39159, as was already an issue with -Werror=attributes in Bug 867636.

	leper Reporter
	Comment 6 • 8 years ago

Adds exception for the include order bug (check_spider_monkey_style.py related failures in https://treeherder.mozilla.org/#/jobs?repo=try&revision=62da6549e95a1072419df7931e22b67748a5de1d ). Otherwise identical to the previous patch.

	leper Reporter
	Comment 7 • 8 years ago

The last attachment was pushed to the try server by arai: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0e19361669bb59d00a3013a9a9e5d784e55b51e .

	Luke Wagner [:luke]
	Comment 8 • 8 years ago

Comment on attachment 8810136 [details] [diff] [review]
Export JSPropertyDesc, mark forward decls with the same attribute, and include the header with the forward decl before the header with the definition (GCC warning/bug)., add exception for this bug

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

This seems pretty unfortunate and, iiuc, likely to cause unexpected (and confusing) bustage for people in the future.  Can we instead remove PropertyDescriptor from NamespaceImports.h?

	leper Reporter
	Comment 9 • 8 years ago

I did think about doing that earlier, but a search did list quite a lot more occurences of PropertyDescriptor, however most of those are in places already `using JS`.

A Try Server run would be nice.

	Luke Wagner [:luke]
	Comment 10 • 8 years ago

Comment on attachment 8810876 [details] [diff] [review]
Stop importing JS::PropertyDescriptor into the js:: namespace.

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

Thanks, this looks much nicer!  Here's a try push:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=15f655bd526c639d151ead781abab53a6de0d70b

::: js/src/jspubtd.h
@@ +146,5 @@
>  namespace JS {
>  
>  class JS_PUBLIC_API(AutoEnterCycleCollection);
>  class JS_PUBLIC_API(AutoAssertOnBarrier);
> +struct JS_PUBLIC_API(PropertyDescriptor);

What's weird is that AutoAssertBarrier does have a JS_PUBLIC_API on its definition in GCAPI.h.

	leper Reporter
	Comment 11 • 8 years ago

Guess I got what I deserve for not checking whether I had any modified but not added files before uploading. Now with Proxy.h changes.

	Luke Wagner [:luke]
	Comment 12 • 8 years ago

Comment on attachment 8810957 [details] [diff] [review]
Actually add all files (Proxy.h was missing).

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

D'oh, it's easy to forget non-unified builds.  New try push with just the failing platforms:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=e701d1d3a0cf2b4453bb49d54e64d0e718083431

	Pulsebot
	Comment 13 • 8 years ago

Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/103f438fb9b2
Mark JS::PropertyDescriptor as JS_PUBLIC_API to fix linking. r=luke

	Phil Ringnalda (:philor)
	Comment 14 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/103f438fb9b2

	areya
	Comment 15 • 8 years ago

https://hg.mozilla.org/mozilla-central/rev/103f438fb9b2

	leper Reporter
	Comment 16 • 8 years ago

Comment on attachment 8810957 [details] [diff] [review]
Actually add all files (Proxy.h was missing).

[Feature/regressing bug #]: 1316079
[User impact if declined]: All embedders that are using PropertyDescriptor will have linker failures due to undefined references caused by wrong symbol visibility. This would require them to patch the used SpiderMonkey to fix this issue or stop using PropertyDescriptor, until SM59 is released. Having this included in FF52 (so and thus ESR52), from which the SM52 release will be rolled would greatly improve actually using the next SpiderMonkey release.
[Describe test coverage new/current, TreeHerder]: Passed automated tests and landed on mozilla-central. Verified to fix the linking issue by building the test case attached to the ticket.
[Risks and why]: Low risk: Only changes the symbol visibility to actually be visible for embedding users.
[String/UUID change made/needed]: None.

	Julien Cristau [:jcristau]
	Comment 17 • 8 years ago

Comment on attachment 8810957 [details] [diff] [review]
Actually add all files (Proxy.h was missing).

fix spidermonkey embedding, take in aurora52

	Carsten Book [:Tomcat]
	Comment 18 • 8 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-aurora/rev/be65a95a6b4b