1027951 - Assignments to EXPORTED_SYMBOLS should not fail silently

Status: RESOLVED INACTIVE

(Core :: XPConnect, defect)

Description

[Sam Penrose]

bug 1027874 is an example of wasted time due to a developer's forgetting that the following block silently fails:

EXPORTED_SYMBOLS = ["X"];
var X = 1;

(the second line must read "this.X = 1"). When it does fail, the symptoms are a missing symbol in the consuming module. Tracking down the source of the symbol, then remembering (or discovering by trial-and-error with a successfully exported symbol) the culprit wastes lots of developer time (today with bug 1027874; I have seen it previously). Whether by having whatever reads EXPORTED_SYMBOLS throw an error or by a separate linting step, we should clean this failure mode up.

Comment 1

[Dave Garrett]

(In reply to Sam Penrose from comment #0)
> EXPORTED_SYMBOLS = ["X"];
> var X = 1;
> 
> (the second line must read "this.X = 1")

No, this should work fine. Based on the patch in bug 1027953 (attachment 8443148 [details] [diff] [review]), your issue was probably that you were defining as a function rather than as a (global) variable. (though, I thought this would've been fine too) e.g.:

bad:
function X() {...}

good:
var X = function() {...};

Also note that your patch probably needs a new semicolon after the function definition now, though JS gets by without it usually.

Nonetheless, I don't disagree with the premise of the bug. Missing symbols in an EXPORTED_SYMBOLS definition really should throw an error.

Comment 2

[Sam Penrose]

Thanks for noting the semicolon. I can cause mach xpcshell-test to fail this way:

0:04.66 TEST-UNEXPECTED-FAIL | /Users/spenrose/repo/hg/mozilla-central/testing/xpcshell/head.js | Unexpected exception TypeError: deriveHawkCredentials is not a function at /Users/spenrose/repo/hg/build_unified_b2g/_tests/xpcshell/services/common/tests/unit/test_hawkrequest.js:218

with this change:

-this.deriveHawkCredentials = function deriveHawkCredentials(tokenHex, context, size=96) {
+var deriveHawkCredentials = function (tokenHex, context, size=96) {

(Leaving the name in between "function " and "(" does not affect the behavior). Please let me know if you cannot reproduce the error I get.

(In reply to Dave Garrett from comment #1)
> (In reply to Sam Penrose from comment #0)
> > EXPORTED_SYMBOLS = ["X"];
> > var X = 1;
> > 
> > (the second line must read "this.X = 1")
> 
> No, this should work fine. Based on the patch in bug 1027953 (attachment
> 8443148 [details] [diff] [review]), your issue was probably that you were
> defining as a function rather than as a (global) variable. (though, I
> thought this would've been fine too) e.g.:
> 
> bad:
> function X() {...}
> 
> good:
> var X = function() {...};
> 
> Also note that your patch probably needs a new semicolon after the function
> definition now, though JS gets by without it usually.
> 
> Nonetheless, I don't disagree with the premise of the bug. Missing symbols
> in an EXPORTED_SYMBOLS definition really should throw an error.

Comment 3

[Dave Garrett]

Attachment: test exported symbols XPI

(In reply to Dave Garrett from comment #1)
> your issue was probably that you were
> defining as a function rather than as a (global) variable. (though, I
> thought this would've been fine too)

This guess was wrong and my initial thought was correct, as defining as a functions should work too.

I put together a quick testcase. See attachment.

All of the following should export fine:
function test1Function() {...}  // old
this.test1FunctionThisProp = function() {...};  // new
var test1FunctionVar = function() {...};  // my suggestion
var test1Object = { FunctionProp : function() {...} };  // my general usage

Both of these work fine with all of the above:
const EXPORTED_SYMBOLS = [...];  // How I generally define it
this.EXPORTED_SYMBOLS = [...];  // How the file in the other bug defined it

I don't know why your code failed, but I do know that it's not because of the form in comment 0.

Test case run on Linux in current nightly & ESR. Check browser console to see which exported symbols work.

Comment 4

[Dave Garrett]

And applying "use strict"; to the files does not affect anything in my testing either.

Comment 5

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Hi, Dave, I'm also interested in this bug and its root cause.

It appears that Sam's one-liner really is required to fix this on b2g.  On digging deeper, I see that the difference in behavior between desktop and b2g stems from Bug 798491.  The effect of jsloader.reuseGlobal is summed up by Philipp von Weitershausen as follows:

    Because Firefox and all other products apart from B2G don't have the
    "jsloader.reuseGlobal" pref enabled, so they still get a new global
    per XPCOM JS file / JSM, which means a global variable is equivalent
    to defining a member on `this`. [1]

Since this behavior still comes as a surprise to seasoned developers, and those writing patches for desktop may not have b2g in mind, I support the request for global assignments to EXPORTED_SYMBOLS to fail loudly.

[1] http://comments.gmane.org/gmane.comp.mozilla.devel.platform/1243

Comment 6

[Dave Garrett]

Thanks for investigating and clarifying. Definitely would be preferable to fail loudly if there's going to be platform oddities like this.

Comment 7

[Bobby Holley (:bholley)]

I'm happy to mentor this if someone wants to propose and implement something sane.

Comment 8

[Sam Penrose]

(In reply to Bobby Holley (:bholley) from comment #7)
> I'm happy to mentor this if someone wants to propose and implement something
> sane.

Great! Can you point me at the code that evaluates EXPORTED_SYMBOLS ?

Comment 9

[Bobby Holley (:bholley)]

(In reply to Sam Penrose from comment #8)
> (In reply to Bobby Holley (:bholley) from comment #7)
> > I'm happy to mentor this if someone wants to propose and implement something
> > sane.
> 
> Great! Can you point me at the code that evaluates EXPORTED_SYMBOLS ?

http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/loader/mozJSComponentLoader.cpp#1300

Comment 10

[BMO Automation]

Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.