618101 - SyntaxError ("let" doesn't work) when loading bootstrap.js to call uninstall() from a disabled add-on

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Ed Lee :Mardak]

Attachment: dummy bootstrapped add-on

If you install the attached add-on then disable it then install it again, you'll get the following warning and uninstall() doesn't run.

Warning: WARN addons.xpi: Error loading bootstrap.js for [e-mail]: SyntaxError: missing ; before statement
Source File: resource://gre/modules/XPIProvider.jsm -> jar:file://../extensions/[e-mail]!/bootstrap.js
Line: 1

bootstrap.js:
let x = 0;
function startup() {}
function shutdown() {}
function install() {}
function uninstall() {}

It seems like whatever is creating the sandbox happens to be using an older version of js somehow.

Comment 1

[Ed Lee :Mardak]

This only happens if the add-on is disabled when installing it again.

If I disable, uninstall (+ reload about:addons), the bootstrap.js loads fine and calls uninstall().

So it seems like there's only a problem when bootstrap.js needs to be loaded for a disabled add-on to call uninstall.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 02/17 - 02/21)]

Edward, please always post the build id you are using. That gives us really helpful information. So let me ask it from you. If you don't use one of the latest minefield builds, please update. Bug 596580 got fixed lately, which could be a symptom. If not, which JS version are we using for bootstrapped add-ons?

Comment 3

[Ed Lee :Mardak]

Build identifier: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b8pre) Gecko/20101209 Firefox/4.0b8pre

Comment 4

[Dave Townsend [:mossop]]

This is probably to do with the creation of the sandbox happening either from an XPCOM component's context (for the normal startup case) or the window context (for the uninstall while disabled case).

Comment 5

[Dave Townsend [:mossop]]

Wait no that is not it at all. This is a confusing one.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 02/17 - 02/21)]

There is still the open bug 613383. Not sure if it could matter for this issue.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Bug 595691 could certainly be related, though it's not clear why we'd ever _downgrade_ the version...

Comment 8

[Dave Townsend [:mossop]]

Mano was going to investigate this

Comment 9

[Ed Lee :Mardak]

Bug 604301 might also be related?

Comment 10

[Asaf Romano (gone)]

We don't run the code through eval or evalInSandbox.

Comment 11

[Dave Townsend [:mossop]]

(In reply to comment #10)
> We don't run the code through eval or evalInSandbox.

What is the status here? Sounds like you have a good idea of how to fix it

Comment 12

[Asaf Romano (gone)]

Attachment: the quicker fix

Here is a one-liner solution. I don't know the javascript-version-autodetect code very well (I used to think chrome just gets "latest"), but cleary the js version for the xpinstall-confirm-dialog is < 1.7 if not specified explicitly.

Soon I'll post the evalInSandbox-based solution, which is somewhat ugly but much more correct IMO. While both solutions aren't risky IMO, I guess smaller is better this late in the game.

Comment 13

[Asaf Romano (gone)]

Attachment: the other option

This patch fixes any potential caller, not just xpinstall. I like it more, you might not.

Unfortunately, both options are bad in the same way: we'll have to update this number every once in a while. :-/

Comment 14

[Dave Townsend [:mossop]]

Comment on attachment 505191 [details] [diff] [review]
the other option

Yeah I prefer this option, feels like it is more likely to cover all the bases, looks like you should use the version "ECMAv5" for now though as the latest version. I've filed bug 627196 on allowing us to just fix the string to something that means latest version here but I think this fix is good enough to ship with.

Is there anyway to test this?

Comment 15

[Asaf Romano (gone)]

Attachment: A test? something along these lines should have worked

But it doesn't, and  I have no idea why.  I suppose we face some sandbox-mess here (maybe I should have created the install object within the sandbox context? I cannot tell how far this goes...).  Anyhow, we shouldn't spent more time on this IMO.

Comment 16

[Dave Townsend [:mossop]]

(In reply to comment #15)
> Created attachment 505237 [details] [diff] [review]
> A test? something along these lines should have worked
> 
> But it doesn't, and  I have no idea why.  I suppose we face some sandbox-mess
> here (maybe I should have created the install object within the sandbox
> context? I cannot tell how far this goes...).  Anyhow, we shouldn't spent more
> time on this IMO.

Yeah you're probably right, let's get it landed

Comment 17

[Asaf Romano (gone)]

Attachment: The patch I'll check in when the tree is open again.

Comment 18

[Asaf Romano (gone)]

http://hg.mozilla.org/mozilla-central/rev/c24246179188

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 02/17 - 02/21)]

Edward, I'm sadly still not able to reproduce this bug in older builds. Would you have a minute to test if this issue is fixed for you now? Thanks.

Comment 20

[Ed Lee :Mardak]

I can reproduce in Firefox 4 Beta 9 using the STR in comment 0. (With the addition of extensions.checkCompatibility.4.0b = false as the original xpi I attached didn't have a maxver including b9.)

Verified fixed on nightlies.