Closed Bug 1213160 Opened 4 years ago Closed 4 years ago

Scan AMO add-ons for code likely to broken by const-scoping changes and new top-level lexical scope

Categories

(addons.mozilla.org Graveyard :: Administration, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: kmag, Assigned: kmag)

References

Details

This is going to be tricky to do correctly. The easiest thing to do is find any add-on with top-level `const` or `let` declarations, and email those authors. That's likely to generate a lot of unnecessary noise, though, and will miss `const` declarations affected by the new temporal dead zone, or bound to an unexpected block scope.


Instead, I'm planning to test for the following:

* References to any undeclared variable which is later declared as a `const` in the same function/top-level scope.

* References to any variable declared as `const` in the same function/top-level scope, but not in the same block scope.

* Any variable declared with a top-level `const` or `let` statement which also:

   - Appears in an EXPORTED_SYMBOLS array.

   - Is accessed on any object returned by `Components.import`.

   - Is accessed on any object that's the target of a `loadSubScript` call.

   - Is accessed as an unbound variable reference.

This should all be pretty easily doable with my local branch of the add-on validator. It will definitely miss some rare corner cases, and probably turn up some false positives, but it should be much less noisy than the alternative.
I sent an email to dev-addons with my results.
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
I've hit one more scenario where this change is problematic - a "global" let variable that is being accessed by an inline event handler in the XUL/HTML document. I guess that your test doesn't detect that? Would be rather tricky.
It currently detects top-level `let` declarations in any script loaded by a XUL overlay. It currently doesn't do any checks for scripts loaded into documents any other way, or for whether those variables are actually touched by other scripts.

I'm hoping to add better tests for cases like that early next week. See https://mail.mozilla.org/pipermail/dev-addons/2015-October/000083.html
In my case it was a dialog and not an overlay - those have the same issue. The add-on in question is Adblock Plus so I guess that your test didn't catch it after all (would have affected your ADU numbers otherwise).
A lot of things have come up over the past couple of weeks. It's taken me a while to get back to this. How does this look?

Warning: Variable may not have been exported
You are attempting to access the variable `Utils` from `a shared global scope`. A variable with this name was with block scoping, elsewhere in your add-on, and may not be accessible here.
Since bug 1202902, variables declared at the top-level using `let` or `const` are attached to a top-level block scope, rather than to the global. Please use `var` instead.
        Tier:   3
        File:   chrome/content/ui/filters.xul
        Line:   386
        Column: 66
        Context:
        >   _restoreVersionWarning="&restore.minVersion.warning;"
        >   oncommand="if (event.target == this) Utils.runAsync(() => this.open = true);">
        > <menupopup onpopupshowing="Backup.fillRestorePopup();">

Warning: Top-level, block-scoped variables not exported
The top-level, block-scoped variable `Utils` has the same name as a variable accessed from a shared global scope elsewhere in your add-on. Variables declared with block scoping are not available via those import methods.
Since bug 1202902, variables declared at the top-level using `let` or `const` are attached to a top-level block scope, rather than to the global. Please use `var` instead.
        Tier:   3
        File:   lib/utils.js
        Line:   30
        Column: 4
        Context:
        >  */
        > let Utils = exports.Utils =
        > {

Warning: Variable may not have been exported
You are attempting to access the variable `advancedMode` from `a shared global scope`. A variable with this name was with block scoping, elsewhere in your add-on, and may not be accessible here.
Since bug 1202902, variables declared at the top-level using `let` or `const` are attached to a top-level block scope, rather than to the global. Please use `var` instead.
        Tier:   3
        File:   chrome/content/ui/composer.xul
        Line:   30
        Column: 26
        Context:
        > ondialogaccept="return addFilter()"
        > ondialogdisclosure="setAdvancedMode(!advancedMode)"
        > buttons="accept,cancel,disclosure"

Warning: Top-level, block-scoped variables not exported
The top-level, block-scoped variable `advancedMode` has the same name as a variable accessed from a shared global scope elsewhere in your add-on. Variables declared with block scoping are not available via those import methods.
Since bug 1202902, variables declared at the top-level using `let` or `const` are attached to a top-level block scope, rather than to the global. Please use `var` instead.
        Tier:   3
        File:   chrome/content/ui/composer.js
        Line:   20
        Column: 4
        Context:
        > let item = null;
        > let advancedMode = false;
        >
Blocks: 1224443
Looks nice. While the first one is a false positive, I didn't mind checking (fairly hidden piece of code, not exercised too often). The second one is indeed the issue I meant in comment 4. If that's the only warnings it produced then it's really not too bad.

Nit: "with this name was with block scoping" => "with this name was declared with block scoping"
(In reply to Wladimir Palant from comment #6) 
> Nit: "with this name was with block scoping" => "with this name was declared
> with block scoping"

Yup, I noticed that just after I posted the comment. Thanks.
There has been another addon bustage condition with the global scope changes. bug 1241429 has been reported about an addon bustage due to variables in browser.js declared using `var` and later on re-declaration using `let`.

This is probably worth a revisit
Flags: needinfo?(kmaglione+bmo)
Product: addons.mozilla.org → addons.mozilla.org Graveyard
Flags: needinfo?(kmaglione+bmo)
You need to log in before you can comment on or make changes to this bug.