Closed Bug 632948 Opened 13 years ago Closed 12 years ago

Passing 'strict' argument to C++ setters will silently break extension that use JSAPI

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED WONTFIX

People

(Reporter: jimb, Assigned: jimb)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Convert JSD to C++.
44.06 KB, patch
Details | Diff | Splinter Review
Export JSAPI functions using C++ linkage, not C linkage.
4.58 KB, patch
Details | Diff | Splinter Review 
Bug 537873 changed the type of C++ setters for JS properties to include a 'strict' flag, needed to implement the assignment semantics required by ES5. However, this fix doesn't cause any change that would prevent binary extensions that use JSAPI from loading; no externally visible symbol's name has changed, for example.

Gal says this is no big deal --- that very few extensions use JSAPI. Indeed, most are pure JS, and most extensions that do have binaries use XPCOM. However, bsmedberg isn't sure that there aren't some very popular extensions that do use JSAPI.

There are several possible solutions:

- Rename some popular function. Say, JS_InitClass could become JS_InitializeClass. This would probably be enough to cause dynamic linking to fail.

- Use extern "C++" linkage for JSAPI, instead of extern "C" linkage. C++ linkage uses linker symbol names that encode the argument types, which would catch some (although not all) of the mismatches that might occur. (Specifically, the type of one of the members of JSClass has changed, but the linker symbols simply refer to JSClass by name.)

- Ignore the problem, on the grounds that no significant extensions will be affected.
We made a generic promise that interfaces (including things like JSAPI) wouldn't be changed past feature freeze on each stable branch. If we need to make an exception, we can probably do that but we'll need to post prominently to the addon community and be prepared to watch crash-stats for potential crashes related to addons which already have a maxversion of 4.0.*.

I like the C++-linkage option, personally.
https://mxr.mozilla.org/addons/ seems to not search the binary bits.  I wonder whether we could at least grep for JS_InitClass and JS_DefineProperty in the binary addons on AMO.  Those should show up as strings if the binaries actually use either one, right?
The most we should do is C++ linkage option. No API shims.

My gut says it's likely that either no one uses the JSPropertyOp signature in binary add-ons we host or care about in a good way, or else only malware does.

/be
Adding Kev/Fligtar/Jorge - gents, do you know of many add-ons or third-party software that reaches into the JSAPI?

I suspect that Brendan is correct, and that JSAPI is seldom-if-ever used by Add-Ons (perhaps Firebug, though?) since it's the API used to embed the JS engine in applications.
I wouldn't expect many add-ons other than perhaps Firebug and Venkman to use JSAPI. I don't remember ever running into any, and I'm pretty sure I would have if one were submitted for review on AMO.

When this is fixed, it's worth mentioning in the Add-ons Blog (I can do that), but I don't think this will be a serious problem.
Remember this is one tiny corner of the JS API, which may or may not be used by any given embedding of the JS engine or add-on that calls into the engine embedded in Gecko.

I just checked firebug sources, and only

sdk/jetpack-sdk-0.7/packages/nsjetpack/components/src/wrapper.cpp

which looks like our jetpack sdk, defines a JSClass.setProperty hook. Other uses of JSClass and JS_DefineProperty are clean. Cc'ing myk.

/be
(In reply to comment #5)
> I wouldn't expect many add-ons other than perhaps Firebug and Venkman to use
> JSAPI. I don't remember ever running into any, and I'm pretty sure I would have
> if one were submitted for review on AMO.

Okay --- let's get bugs filed against those, then.

> When this is fixed, it's worth mentioning in the Add-ons Blog (I can do that),
> but I don't think this will be a serious problem.

The breaking change will appear the next time we do a TraceMonkey -> Mozilla Central merge.
Firebug and venkman do not have binary blobs.

The only addons that can use JSAPI are those that have binary blobs.

Can someone with access to the addons do the check I asked for in comment 2, please?  That way we'll have some data instead of echo-chamber "nah, no one would do that" comments (which are likely correct, even, but it seems like data would be somewhat better than guessing!).
(In reply to comment #6)
> I just checked firebug sources, and only
> 
> sdk/jetpack-sdk-0.7/packages/nsjetpack/components/src/wrapper.cpp
> 
> which looks like our jetpack sdk, defines a JSClass.setProperty hook. Other
> uses of JSClass and JS_DefineProperty are clean. Cc'ing myk.

Add-on SDK no longer uses packages/nsjetpack/, and we removed it from our repository in bug 610507, so this change is not a problem for Add-on SDK (although I can't speak for Firebug's use of an older version of the SDK).
Actually, I somewhat lied.  Obviously someone could use jsapi without a binary blob via ctypes.  But that would hopefully show up in the addons mxr, and I don't see any obvious hits there.
(In reply to comment #6)
> Other
> uses of JSClass and JS_DefineProperty are clean. Cc'ing myk.

By "clean", you mean, "only use JS_PropertyStub, and are thus okay because it's safe to call JS_PropertyStub with more arguments than it expects"?

I believe ordinary (non-member) C++ functions use __cdecl, in which the callee is responsible for popping arguments, in which case that's correct.
If I'm following:

1) We haven't identified any extensions that could be affected by this;
2) Jorge will blog about the change, just in case; and
3) It would be nice to investigate switching to C++ linkage for JSAPI, within Firefox.

Have I missed any other action items?

Jorge, you may find valuable background in this post:
http://groups.google.com/group/mozilla.dev.tech.js-engine/browse_thread/thread/e6e43da083f28a8e#
> 1) We haven't identified any extensions that could be affected by this;

Well, so far we haven't tried to, right?  Trying seems to be an action item...
bz points out:

4) We need a volunteer to grep for JS_InitClass and JS_DefineProperty in the
binary addons on AMO. (Answering questions through observation of the external world, how non-Aristotelian!) Those should show up as strings if the binaries actually use either one.
(In reply to comment #11)
> (In reply to comment #6)
> > Other
> > uses of JSClass and JS_DefineProperty are clean. Cc'ing myk.
> 
> By "clean", you mean, "only use JS_PropertyStub, and are thus okay because it's
> safe to call JS_PropertyStub with more arguments than it expects"?

By clean I meant:

* JS_DefineProperty calls pass NULL, NULL for getter, setter.
* JS_PropertyStub usage without calling, only as function address.
* Exception: the defunct sdk/jetpack-sdk-0.7/packages/nsjetpack/components/src/wrapper.cpp file, which would break if it were being used.

/be
Blocks: 633178
> 4) We need a volunteer to grep for JS_InitClass and JS_DefineProperty in the
> binary addons on AMO. (Answering questions through observation of the external
> world, how non-Aristotelian!) Those should show up as strings if the binaries
> actually use either one.

Are there any volunteers to do this amongst the folks with the right AMO access?
To make JSAPI use C++ linkage, wouldn't jsd need to be converted to C++? Doing so might not take too long, but it seems to play games with pointer types and such.
Beyond renaming the files and fixing up the makefile, this patch:

- Removes various uses of JS_BEGIN_EXTERN_C and JS_END_EXTERN_C;

- Uses 'JSDStaticLock *' where appropriate, instead of 'void *' (since all uses are in |#ifdef JSD_THREADSAFE| blocks anyway);

- Moves the static declaration of jsd_DebugErrorHook into the file in which it is defined (since that's the only place it can be used anyway);

- Uses JS_StrictPropertyStub instead of JS_PropertyStub, where required;

- Deletes various unused variables and functions (in particular, the JSD_* threading functions);

- Fixes code that tries to use a JSString * as a char *;

- Casts memory returned by allocation functions;

- Deletes jsd_java.c and jsdstubs.c, as these are unused; and

- Adds casts to make the extension of SpiderMonkey's JSTrapStatus enum with new JSD_HOOK_RETURN_* values works with the C++ type system.
Assignee: general → jimb
Remove various uses of JS_BEGIN_EXTERN_C and JS_END_EXTERN_C.

Move the definitions of the CTypes functions declared in jsapi.h out of the
js::ctypes:: namespace.
please split these into distinct commits:

- Fixes code that tries to use a JSString * as a char *;

- Uses 'JSDStaticLock *' where appropriate, instead of 'void *' (since all uses are in |#ifdef JSD_THREADSAFE| blocks anyway);

- Moves the static declaration of jsd_DebugErrorHook into the file in which it is defined (since that's the only place it can be used anyway);

- Uses JS_StrictPropertyStub instead of JS_PropertyStub, where required;

- Casts memory returned by allocation functions;

- Deletes various unused variables and functions (in particular, the JSD_* threading functions);

- Deletes jsd_java.c and jsdstubs.c, as these are unused; and

- Removes various uses of JS_BEGIN_EXTERN_C and JS_END_EXTERN_C;

- Adds casts to make the extension of SpiderMonkey's JSTrapStatus enum with new JSD_HOOK_RETURN_* values work with the C++ type system.

I think that's roughly the right order (note that i've removed an 's' from 'works' in the last item's description).

also, the changes to do this should be in a bug in the jsd component, not hidden in a bug in spidermonkey. you can use one bug for all of those changes, and it should block the bug you use for the spidermonkey changes.
it would also be nice if the ctypes change were done in its own bug in the js-ctypes component, again, blocking the js change, assuming that it's independent, which it appears to be.
(In reply to comment #20)
> also, the changes to do this should be in a bug in the jsd component, not
> hidden in a bug in spidermonkey. you can use one bug for all of those changes,
> and it should block the bug you use for the spidermonkey changes.

Don't worry, these are just a draft, no r requested. Certainly I'll make sure the patch gets reviewed by someone who knows jsd.
(In reply to comment #20)
> please split these into distinct commits:

Will splitting this into eight different patches really be that helpful? The changes aren't very involved.
The link fails on Windows, and I don't understand why. These aren't the only symbols from jsapi.cpp that jsworkers.cpp uses; for example, it cheerfully uses JS_DefineProperty.

link -NOLOGO -OUT:js.exe -PDB:js.pdb  -LARGEADDRESSAWARE -NXCOMPAT -DYNAMICBASE -SAFESEH  -DEBUG -DEBUGTYPE:CV -MANIFEST:NO -LIBPATH:"e:/builds/moz2_slave/try-w32/build/obj-firefox/memory/jemalloc/crtsrc/build/intel" -NODEFAULTLIB:msvcrt -NODEFAULTLIB:msvcrtd -NODEFAULTLIB:msvcprt -NODEFAULTLIB:msvcprtd -DEFAULTLIB:mozcrt19 -DEFAULTLIB:mozcpp19 -DEBUG -OPT:REF -LTCG:PGINSTRUMENT   js.obj jsworkers.obj  e:/builds/moz2_slave/try-w32/build/obj-firefox/dist/lib/nspr4.lib e:/builds/moz2_slave/try-w32/build/obj-firefox/dist/lib/plc4.lib e:/builds/moz2_slave/try-w32/build/obj-firefox/dist/lib/plds4.lib @../js_static.lib.fake kernel32.lib user32.lib gdi32.lib winmm.lib wsock32.lib advapi32.lib
   Creating library js.lib and object js.exp
jsworkers.obj : error LNK2001: unresolved external symbol "unsigned long __cdecl JS_GetOptions(struct JSContext *)" (?JS_GetOptions@@YAKPAUJSContext@@@Z)
jsworkers.obj : error LNK2001: unresolved external symbol "int __cdecl JS_ReadStructuredClone(struct JSContext *,unsigned __int64 const *,unsigned int,unsigned long,unsigned __int64 *,struct JSStructuredCloneCallbacks const *,void *)" (?JS_ReadStructuredClone@@YAHPAUJSContext@@PB_KIKPA_KPBUJSStructuredCloneCallbacks@@PAX@Z)
jsworkers.obj : error LNK2001: unresolved external symbol "int __cdecl JS_SetReservedSlot(struct JSContext *,struct JSObject *,unsigned long,unsigned __int64)" (?JS_SetReservedSlot@@YAHPAUJSContext@@PAUJSObject@@K_K@Z)
jsworkers.obj : error LNK2001: unresolved external symbol "int __cdecl JS_GetReservedSlot(struct JSContext *,struct JSObject *,unsigned long,unsigned __int64 *)" (?JS_GetReservedSlot@@YAHPAUJSContext@@PAUJSObject@@KPA_K@Z)
jsworkers.obj : error LNK2001: unresolved external symbol "void __cdecl JS_CallTracer(struct JSTracer *,void *,unsigned long)" (?JS_CallTracer@@YAXPAUJSTracer@@PAXK@Z)
jsworkers.obj : error LNK2001: unresolved external symbol "unsigned long __cdecl JS_SetOptions(struct JSContext *,unsigned long)" (?JS_SetOptions@@YAKPAUJSContext@@K@Z)
js.exe : fatal error LNK1120: 6 unresolved externals
yes, having patches for individual changes does make things easier, yes.
(In reply to comment #25)
> yes, having patches for individual changes does make things easier, yes.

Seems like busywork. What's the benefit? We don't ordinarily do this.

/be
We do ordinarily do this, since we switched to hg, for large layout/dom changes.

The benefits are often ease of review, ease of bisection if cleanup and substantive changes are kept separate, and ease of patch iteration in some cases.

But usually you develop the patch that way to start with.  Splitting up afterward is a huge PITA, so unless the patch is unreviewable due to the size as-is it's not worth it.
note that patches from js/jsd have a track record of triggering regressions of late. further the reviews we've had to the jsd side have failed to protect against them. limiting to individual changes makes the reviews easier to manage and should reduce these risks.

note that hg qrecord should make peeling most if not all of these changes into individual sets relatively easy to do, but only the patch author is in a good position to ensure they're properly split and thus enable safer reviews.
It would take me about ten minutes to split that patch. I'm afraid folks have spent more time than that commenting on this bug, so it may not have been a win to even complain.

But, look, some patches are really not that hard to look through and see what's going on. This patch is fixing a bunch of *compilation errors* that the C++ compiler found, and it definitely falls in the category where someone with timeless's skills can decompose it easily. I assumed (perhaps in the "ass of u and me" sense) that he just looked at the laundry list I put in the patch comment and said, "Yuck", without looking carefully at how bad it really was.

I will split the patch, without further fuss.
Blocks: 603503
No longer blocks: 603503
The change we were worried would cause compatibility problems has now been in the tree for quite a long time, so I don't think this is worth the investment any more.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.