774636 - Still expose Components until cfx detects all such usages

Status: RESOLVED FIXED

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

Description

[Alexandre Poirot [:ochameau]]

Starting with 1.9, `Components` disappear from global scope.
I think it would be better to first deprecate it with warning message before removing it. It sounds quite important if we want to ease automatic repacking.

The removal was introduced by this changeset:
https://github.com/mozilla/addon-sdk/commit/b167ac2bf0b777bcf5c13ca5735e92d3d8bb666c

For 1.9, we may either avoid using `wantComponents` flag and keep exact same behavior than before, or introduce deprecation message.

Comment 1

[Alexandre Poirot [:ochameau]]

Actually it is slightly more subtle.
Since couple of releases, python already throw errors if it detect any Components usage. So that removing Components shouldn't change anything as it should already fails during cfx run/test/xpi.

But still, there is something wrong. cfx wasn't able to detect following Components usage:
  const Cu = Components.utils;

So that the repack goes wrong with incomprehensible error.

Repacking is already painfull/complex, we should do our best to ease it.

davehunt is currently working on "simply" repacking memchaser addon to a recent version of the SDK:
1/ Dave want to build a version which supports Release, Aurora and Nightly. But 1.8.1 is currently broken because of bug 764688. 
=> It is really hard to understand that current SDK release can easily fail on Nightly/Aurora

2/ Then I suggested him to use 1.9b1, but he face this bug.
=> "ReferenceError: Components is not defined" exception is hard to understand.

On second thought, I think that it is fine to drop this and avoid automatic repacking, but it is really important to be able to tell to addon developers what is wrong and how to address it.
So that it looks like we should keep exposing Components until we fix cfx to detect all usages.

Comment 2

[Alexandre Poirot [:ochameau]]

Here is a potential patch
https://github.com/ochameau/addon-sdk/compare/bug/774636-dont-use-wantComponents

Comment 3

[Alexandre Poirot [:ochameau]]

FYI, they are using Components.utils because our Cu is broken (bug 683217)

https://github.com/mozilla/memchaser/blob/master/extension/lib/main.js#L7-10
  // We have to declare it ourselves because the SK doesn't export it correctly
  const Cu = Components.utils;

  Cu.import('resource://gre/modules/Services.jsm');

Comment 4

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Ok it feels like we have:

1. problem with CFX that does not throws early error if it spots Components.utils
2. `Cu.import` does not works ?

If that's a case we should fix these bugs and leave Components alone. Addons that access `Components.*` directly will be broken if not in this release then in next
one.

Comment 5

[Jeff Griffiths (:canuckistani) (:⚡︎)]

We can discuss in today's meeting, but I agree, IMO Cu.import should work as expected, or we should revisit some higher-level method for importing jsm modules that does the *right thing*.

Comment 6

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

So I have submitted bug 774868 to document behavioral difference of Cu.import in SDK. I have attached patch to bug 683217 preventing Cu.import from polluting loaders global scope. It also looks like cfx is not complaining about `Cu.import` if require('chrome') is present and that's intentional: https://github.com/Gozala/addon-sdk/blob/master/python-lib/cuddlefish/manifest.py#L725-728 more details about why can be found under bug 663541

Comment 7

[Alexandre Poirot [:ochameau]]

Let's avoid discussion about Cu.import here. That's another topic we can discuss in bug 683217.

I've opened this bug in order to highlight the issue introduced by following revision:
https://github.com/mozilla/addon-sdk/commit/b167ac2bf0b777bcf5c13ca5735e92d3d8bb666c
Which memchaser guys faced during repacking.
After some greps on AMO addons, I can see at least 42 addons using Components.
Notable ones are openwebapps, memchaser, remo-companion and librejs.

Comment 8

[Alexandre Poirot [:ochameau]]

So the issue I'd like to see being adressed in 1.9 would be to avoid painfull repacking. If we don't do anything, these 42 addons will fails when being repacked with following runtime exception:
  ReferenceError: Components is not defined

I first suggested to avoid removing Components as a simple unharmfull modification for 1.9 branch. And then remove it properly for 1.10.
But we can keep it removed and tune cfx instead.

I can easily agree that this pratice should be avoided, so that the repack can fail in newer SDK version, but it would be cool to have a understandable error message. The scan of Components.* usages was done in this purpose, we are just facing some edge cases.

What do you think about forbidding all usages of `Components` ? Even if the module use `require("chrome")` ? 
(we can still use lowercase: let {components}=require("chrome"))
So that the repack will fail on build time with explicit error.

Actually I think that we will get in troubles if we suggest using `Components` as it will fail if the addon user is using Firefox version before FF15 (bug 747434)

Or we can keep removal of Components in globals for next release, until most users are are on FF15+ and then remove it while suggesting to use let {Components} = require("chrome");

Last but not least, we can only remove Components from globals for modules that doesn't require chrome.

Comment 9

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

I would go for making python to fail whenever it spots `Components.` regardless of weather 'chrome' is required or not. Yes that won't allow us to repack add-on's that use `Components' but we won't be able to repack them either way unless the code will change.

Comment 10

[Alexandre Poirot [:ochameau]]

Attachment: Pull request 505

Implementation of Comment 9.

Comment 11

[[github robot]]

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

https://github.com/mozilla/addon-sdk/commit/cf278faecfe9d129f78907d51e11d5051432cdb4
Bug 774636: Reject all usages of `Components` in favor of require(chrome).

https://github.com/mozilla/addon-sdk/commit/a6c3f415b262049d0c04c6afc04aad481e4fa587
Merge pull request #505 from ochameau/bug/774636-reject-usages-of-components

Bug 774636: reject all usages of `Components` in favor of require(chrome) r=@gozala

Comment 12

[Alexandre Poirot [:ochameau]]

Wes, can you pull this change in 1.9?

Comment 13

[[github robot]]

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

https://github.com/mozilla/addon-sdk/commit/377d6c5b94e252c8c667f46edb6950b0f43d84b0
Merge pull request #505 from ochameau/bug/774636-reject-usages-of-components

Bug 774636: reject all usages of `Components` in favor of require(chrome) r=@gozala(cherry picked from commit a6c3f415b262049d0c04c6afc04aad481e4fa587)

Comment 14

[[github robot]]

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

https://github.com/mozilla/addon-sdk/commit/377d6c5b94e252c8c667f46edb6950b0f43d84b0
Merge pull request #505 from ochameau/bug/774636-reject-usages-of-components

Comment 15

[[github robot]]

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

https://github.com/mozilla/addon-sdk/commit/cf278faecfe9d129f78907d51e11d5051432cdb4
Bug 774636: Reject all usages of `Components` in favor of require(chrome).

https://github.com/mozilla/addon-sdk/commit/a6c3f415b262049d0c04c6afc04aad481e4fa587
Merge pull request #505 from ochameau/bug/774636-reject-usages-of-components

Comment 16

[[github robot]]

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

https://github.com/mozilla/addon-sdk/commit/cf278faecfe9d129f78907d51e11d5051432cdb4
Bug 774636: Reject all usages of `Components` in favor of require(chrome).

https://github.com/mozilla/addon-sdk/commit/a6c3f415b262049d0c04c6afc04aad481e4fa587
Merge pull request #505 from ochameau/bug/774636-reject-usages-of-components