855651 - Add-on SDK 1.14 refuses iceweasel

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect, P3)

Description

[Kusanagi Kouichi]

Attachment: Accept re-brended products

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:19.0) Gecko/20100101 Firefox/19.0 Iceweasel/19.0.2
Build ID: 20130308220207

Steps to reproduce:

Run the following code with add-on sdk 1.14 and iceweasel:

require("sdk/tabs");
console.log("ok");


Actual results:

It threw an exception as follows:

error: aaa: An exception occurred.
Error: Unsupported Application: The module sdk/tabs/tabs currently supports only Firefox, Fennec.
resource://jid1-rpfyi5ceseitqa-at-jetpack/addon-sdk/lib/sdk/loader/cuddlefish.js 80
Traceback (most recent call last):
  File "resource://gre/modules/NetUtil.jsm", line 140, in
    aCallback(pipe.inputStream, aStatusCode, aRequest);
  File "resource://jid1-rpfyi5ceseitqa-at-jetpack/addon-sdk/lib/sdk/net/url.js", line 49, in
    resolve(data);
  File "resource://jid1-rpfyi5ceseitqa-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 143, in resolve
    while (pending.length) result.then.apply(result, pending.shift())
  File "resource://jid1-rpfyi5ceseitqa-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 37, in then
    return { then: function then(resolve) { resolve(value) } }
  File "resource://jid1-rpfyi5ceseitqa-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 117, in resolved
    function resolved(value) { deferred.resolve(resolve(value)) }
  File "resource://jid1-rpfyi5ceseitqa-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 143, in resolve
    while (pending.length) result.then.apply(result, pending.shift())
  File "resource://jid1-rpfyi5ceseitqa-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 37, in then
    return { then: function then(resolve) { resolve(value) } }
  File "resource://jid1-rpfyi5ceseitqa-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 117, in resolved
    function resolved(value) { deferred.resolve(resolve(value)) }
  File "resource://jid1-rpfyi5ceseitqa-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 143, in resolve
    while (pending.length) result.then.apply(result, pending.shift())
  File "resource://jid1-rpfyi5ceseitqa-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 37, in then
    return { then: function then(resolve) { resolve(value) } }
  File "resource://jid1-rpfyi5ceseitqa-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 117, in resolved
    function resolved(value) { deferred.resolve(resolve(value)) }
  File "resource://jid1-rpfyi5ceseitqa-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 143, in resolve
    while (pending.length) result.then.apply(result, pending.shift())
  File "resource://jid1-rpfyi5ceseitqa-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 37, in then
    return { then: function then(resolve) { resolve(value) } }
  File "resource://jid1-rpfyi5ceseitqa-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 117, in resolved
    function resolved(value) { deferred.resolve(resolve(value)) }
  File "resource://jid1-rpfyi5ceseitqa-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 143, in resolve
    while (pending.length) result.then.apply(result, pending.shift())
  File "resource://jid1-rpfyi5ceseitqa-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 37, in then
    return { then: function then(resolve) { resolve(value) } }
  File "resource://jid1-rpfyi5ceseitqa-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 117, in resolved
    function resolved(value) { deferred.resolve(resolve(value)) }
  File "resource://jid1-rpfyi5ceseitqa-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 143, in resolve
    while (pending.length) result.then.apply(result, pending.shift())
  File "resource://jid1-rpfyi5ceseitqa-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 123, in then
    else result.then(resolved, rejected)
  File "resource://jid1-rpfyi5ceseitqa-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 37, in then
    return { then: function then(resolve) { resolve(value) } }
  File "resource://jid1-rpfyi5ceseitqa-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 117, in resolved
    function resolved(value) { deferred.resolve(resolve(value)) }
  File "resource://jid1-rpfyi5ceseitqa-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 55, in effort
    try { return f(options) }
  File "resource://jid1-rpfyi5ceseitqa-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 117, in resolved
    function resolved(value) { deferred.resolve(resolve(value)) }
  File "resource://jid1-rpfyi5ceseitqa-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 143, in resolve
    while (pending.length) result.then.apply(result, pending.shift())
  File "resource://jid1-rpfyi5ceseitqa-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 37, in then
    return { then: function then(resolve) { resolve(value) } }
  File "resource://jid1-rpfyi5ceseitqa-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 117, in resolved
    function resolved(value) { deferred.resolve(resolve(value)) }
  File "resource://jid1-rpfyi5ceseitqa-at-jetpack/addon-sdk/lib/sdk/core/promise.js", line 55, in effort
    try { return f(options) }
  File "resource://jid1-rpfyi5ceseitqa-at-jetpack/addon-sdk/lib/sdk/addon/runner.js", line 90, in onLocalizationReady
    run(options);
  File "resource://jid1-rpfyi5ceseitqa-at-jetpack/addon-sdk/lib/sdk/addon/runner.js", line 122, in run
    let program = main(options.loader, options.main);
  File "resource://jid1-rpfyi5ceseitqa-at-jetpack/aaa/lib/main.js", line 1, in
    require("sdk/tabs");
  File "resource://jid1-rpfyi5ceseitqa-at-jetpack/addon-sdk/lib/sdk/loader/cuddlefish.js", line 127, in options<.load
    result = load(loader, module);
  File "resource://jid1-rpfyi5ceseitqa-at-jetpack/addon-sdk/lib/sdk/tabs.js", line 10, in
    module.exports = require('./tabs/tabs');
  File "resource://jid1-rpfyi5ceseitqa-at-jetpack/addon-sdk/lib/sdk/loader/cuddlefish.js", line 133, in options<.load
    error = incompatibility(module) || error;
  File "resource://jid1-rpfyi5ceseitqa-at-jetpack/addon-sdk/lib/sdk/loader/cuddlefish.js", line 80, in incompatibility
    " currently supports only " + applications.join(", ") + ".")


Expected results:

Print "ok"

Comment 1

[Matteo Ferretti [:zer0] [:matteo]]

The patch is actually remove the feature the introduce by the metadata properties; where we want to throw an exception if the current application is not explicitly listed in the module.

What we could do, is let the add-on devs to decide if they are interested in those warning or not, or defines an alias.

So, for instance, they could say "Hey, for my add-on the application called 'Foo' should be thread as alias for 'Firefox' during the module's compatibility check".

I prefer this solution instead of disabling the check at all, because if some module are not supported by Firefox – let's say, some third party module created just for Thunderbird or Fennec – then the devs still get the error message.

That could be done probably in harness-options from command line, or set a new attribute on package.json; in the latter case we could probably combine that with bug 724276.

Comment 2

[Matteo Ferretti [:zer0] [:matteo]]

Kusanagi Kouichi, do you think is the aliases a reasonable approach that could solve your use case?

Comment 3

[Kusanagi Kouichi]

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #2)
> Kusanagi Kouichi, do you think is the aliases a reasonable approach that
> could solve your use case?

Why do we need to check app's name? What do you think about the following comment in lib/sdk/system/xul-app.js:

// Using the GUID instead of the app's name is preferable because sometimes
// re-branded versions of a product have different names: for instance,
// Firefox, Minefield, Iceweasel, and Shiretoko all have the same
// GUID.

Comment 4

[Matteo Ferretti [:zer0] [:matteo]]

Sorry my bad, I didn't actually remember that the GUID for rebranded products are actually shared; so I misunderstood your patch and over thinking the issue.

Comment 5

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 730615 [details] [diff] [review]
Accept re-brended products

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

::: lib/sdk/loader/cuddlefish.js
@@ +64,5 @@
>    let applications = Object.keys(engines);
>  
> +  let versionRange;
> +  applications.forEach(function(name) {
> +    if (xulappModule.is(name)) {

Just add a comment here, that saying that we're continuing the iteration even if we found the version for the current one, because we want to ensure the module doesn't contain a typo in the applications' name or some unknown application – `is` function throw an exception in that case.

Comment 6

[Matteo Ferretti [:zer0] [:matteo]]

It's a bit odd makes the validation in that way, it seems a bit misleading – but you just did what was done before. If you could came up with a better idea your're more than welcome, otherwise it's fine as is.

Comment 7

[Kusanagi Kouichi]

Attachment: Accept re-brended products v2

How abowt this?

Comment 8

[Kusanagi Kouichi]

Attachment: Accept re-brended products v3

V2 is wrong. We should test all names and shouldn't catch an exception from "is". Right?

Comment 10

[zzenitt]

I'm experiencing the same bug. I'm developing an addon, I upgraded the SDK to 1.14(using Mozilla Addon Builder) and now I can't test my addon anymore(Addon Builder doesn't allow to downgrade the SDK version).

The V3 patch of Kusanagi Kouichi looks good.

Somebody know some workaround to be able to use Mozilla Addon Builder with Iceweasel and SDK 1.14, and be able to build and test the addon?

I can download the sources, patch the SDK and then install the XPI but I need something faster, I can't do that to test every thing that I need to test.

Thanks!

Comment 11

[Matteo Ferretti [:zer0] [:matteo]]

Kusanagi, I will keep the V1 with the comment you added in the V3 if it's fine to you. We'll could improve that function later on (I'd like to avoid the `for…in` because it iterates all the enumerable properties, where the `Object.keys` will returns only the owns).

Comment 12

[Kusanagi Kouichi]

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #11)
> Kusanagi, I will keep the V1 with the comment you added in the V3 if it's
> fine to you. We'll could improve that function later on (I'd like to avoid
> the `for…in` because it iterates all the enumerable properties, where the
> `Object.keys` will returns only the owns).

V1 + comment is fine with me.

Comment 13

[Matteo Ferretti [:zer0] [:matteo]]

Attachment: Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/957/files

Pointer to Github pull-request

Comment 14

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 739970 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/957/files

Not sure if I have to create a pull request for a patch from a contributor that is reviewed on bugzilla; I just played safe to keep track of it.

Comment 15

[[github robot]]

Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/18b7544c08b3711118dc389765430ba1e3a02d84
Bug 855651 - Add-on SDK 1.14 refuses iceweasel

https://github.com/mozilla/addon-sdk/commit/d7f260cdf8e9ca8fa0b6491ad57dd52ba6672f48
Merge pull request #957 from ZER0/rebranded/855651

fix Bug 855651 - Add-on SDK 1.14 refuses iceweasel r=@ZER0

Comment 17

[[github robot]]

Commits pushed to integration at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/18b7544c08b3711118dc389765430ba1e3a02d84
Bug 855651 - Add-on SDK 1.14 refuses iceweasel

https://github.com/mozilla/addon-sdk/commit/d7f260cdf8e9ca8fa0b6491ad57dd52ba6672f48
Merge pull request #957 from ZER0/rebranded/855651

Comment 18

[Dave Townsend [:mossop]]

Attachment: beta/aurora patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Unknown
User impact if declined: SDK based add-ons won't work on Firefox derivatives like Pale Moon or Ice Weasel
Testing completed (on m-c, etc.): In Nightlies for a few days
Risk to taking this patch (and alternatives if risky): Very low risk, Firefox itself doesn't use this code right now.
String or IDL/UUID changes made by this patch: None

Comment 19

[bhavana bajaj [:bajaj]]

(In reply to Dave Townsend (:Mossop) from comment #18)
> Created attachment 743768 [details] [diff] [review]
> beta/aurora patch
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): Unknown
> User impact if declined: SDK based add-ons won't work on Firefox derivatives
> like Pale Moon or Ice Weasel
> Testing completed (on m-c, etc.): In Nightlies for a few days
> Risk to taking this patch (and alternatives if risky): Very low risk,
> Firefox itself doesn't use this code right now.
> String or IDL/UUID changes made by this patch: None

Do we know what versions of Firefox are affected here ? If Fx20 is affected I would like to maintain the status quo for Fx21 and uplift this only until aurora .Or we do we have strong reasons for this being a recent user critical regression ?

Comment 20

[bhavana bajaj [:bajaj]]

(In reply to bhavana bajaj [:bajaj] from comment #19)
> (In reply to Dave Townsend (:Mossop) from comment #18)
> > Created attachment 743768 [details] [diff] [review]
> > beta/aurora patch
> > 
> > [Approval Request Comment]
> > Bug caused by (feature/regressing bug #): Unknown
> > User impact if declined: SDK based add-ons won't work on Firefox derivatives
> > like Pale Moon or Ice Weasel
> > Testing completed (on m-c, etc.): In Nightlies for a few days
> > Risk to taking this patch (and alternatives if risky): Very low risk,
> > Firefox itself doesn't use this code right now.
> > String or IDL/UUID changes made by this patch: None
> 
> Do we know what versions of Firefox are affected here ? If Fx20 is affected
> I would like to maintain the status quo for Fx21 and uplift this only until
> aurora .Or we do we have strong reasons for this being a recent user
> critical regression ?

I would like to add, that although this low risk we are only left with our final two beta's with our second last beta going to build today hence the concern here.

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(In reply to bhavana bajaj [:bajaj] from comment #19)
> Do we know what versions of Firefox are affected here ? If Fx20 is affected
> I would like to maintain the status quo for Fx21 and uplift this only until
> aurora .Or we do we have strong reasons for this being a recent user
> critical regression ?

The regression landed and shipped in SDK 1.14, which is also being shipped in Firefox 21 and 22, while the fix for the regression is now in for 23.

Comment 22

[bhavana bajaj [:bajaj]]

Comment on attachment 743768 [details] [diff] [review]
beta/aurora patch

Approving as this Add-on SDK version will be shipping with FX21 and is recently released to avoid the any user annoyance this may cause among users who are building add-ons on firefox derivates .

Please add qawanted if there is any explicit qa testing needed on our side.

And request to land this on beta asap so this can get into our second last beta going to build soon.

Comment 23

[Dave Townsend [:mossop]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/433557ff42dc
https://hg.mozilla.org/releases/mozilla-beta/rev/223f5ada4b4e

Comment 24

[[github robot]]

Commit pushed to firefox21 at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/50670c29c118fc69dd24269d51754f400b3306d4
Bug 855651: Add-on SDK 1.14 refuses iceweasel. r=ZER0, a=bajaj

Comment 25

[[github robot]]

Commit pushed to firefox22 at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/a759a2a68d0166ef3f91d9361cd40f26b0833580
Bug 855651: Add-on SDK 1.14 refuses iceweasel. r=ZER0, a=bajaj