Last Comment Bug 855651 - Add-on SDK 1.14 refuses iceweasel
: Add-on SDK 1.14 refuses iceweasel
Status: RESOLVED FIXED
:
Product: Add-on SDK
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All All
: P3 normal with 1 vote (vote)
: ---
Assigned To: Kusanagi Kouichi
:
Mentors:
: 861826 864933 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-03-28 04:09 PDT by Kusanagi Kouichi
Modified: 2013-07-01 15:29 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed
fixed


Attachments
Accept re-brended products (616 bytes, patch)
2013-03-28 04:09 PDT, Kusanagi Kouichi
zer0: review+
Details | Diff | Review
Accept re-brended products v2 (788 bytes, patch)
2013-04-13 21:48 PDT, Kusanagi Kouichi
no flags Details | Diff | Review
Accept re-brended products v3 (934 bytes, patch)
2013-04-14 06:16 PDT, Kusanagi Kouichi
no flags Details | Diff | Review
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/957/files (373 bytes, text/html)
2013-04-20 08:08 PDT, Matteo Ferretti [:zer0] [:matteo]
zer0: review+
Details
beta/aurora patch (1011 bytes, patch)
2013-04-30 11:57 PDT, Dave Townsend [:mossop]
dtownsend: review+
bajaj.bhavana: approval‑mozilla‑aurora+
bajaj.bhavana: approval‑mozilla‑beta+
Details | Diff | Review

Description Kusanagi Kouichi 2013-03-28 04:09:02 PDT
Created attachment 730615 [details] [diff] [review]
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] 2013-04-12 04:32:59 PDT
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] 2013-04-12 04:35:43 PDT
Kusanagi Kouichi, do you think is the aliases a reasonable approach that could solve your use case?
Comment 3 Kusanagi Kouichi 2013-04-13 01:18:16 PDT
(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] 2013-04-13 09:12:28 PDT
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] 2013-04-13 09:14:15 PDT
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] 2013-04-13 09:18:50 PDT
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 2013-04-13 21:48:20 PDT
Created attachment 737208 [details] [diff] [review]
Accept re-brended products v2

How abowt this?
Comment 8 Kusanagi Kouichi 2013-04-14 06:16:54 PDT
Created attachment 737251 [details] [diff] [review]
Accept re-brended products v3

V2 is wrong. We should test all names and shouldn't catch an exception from "is". Right?
Comment 9 Wes Kocher (:KWierso) 2013-04-15 08:26:30 PDT
*** Bug 861826 has been marked as a duplicate of this bug. ***
Comment 10 zzenitt 2013-04-18 21:11:24 PDT
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] 2013-04-19 18:13:00 PDT
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 2013-04-19 22:59:56 PDT
(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] 2013-04-20 08:08:08 PDT
Created attachment 739970 [details]
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] 2013-04-20 08:09:49 PDT
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] 2013-04-20 08:10:31 PDT
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 16 Matteo Ferretti [:zer0] [:matteo] 2013-04-24 00:38:29 PDT
*** Bug 864933 has been marked as a duplicate of this bug. ***
Comment 17 [github robot] 2013-04-26 01:59:12 PDT
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] 2013-04-30 11:57:29 PDT
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
Comment 19 bhavana bajaj [:bajaj] 2013-04-30 12:29:34 PDT
(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] 2013-04-30 12:31:25 PDT
(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) 2013-04-30 13:38:00 PDT
(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] 2013-04-30 14:17:24 PDT
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 24 [github robot] 2013-07-01 15:29:07 PDT
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] 2013-07-01 15:29:26 PDT
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

Note You need to log in before you can comment on or make changes to this bug.