Closed Bug 1283622 Opened 8 years ago Closed 8 years ago

more devtools signing failures

Categories

(Toolkit :: Add-ons Manager, defect)

Product:
Toolkit
Component:
Add-ons Manager
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla50
Tracking Flags:
Tracking Status
firefox50 --- fixed

People

(Reporter: andy+bugzilla, Assigned: aswan)

References

Details

Attachments

(1 obsolete file) 

In: http://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-beta-win64-debug/1467228686/mozilla-beta_win8_64-debug_test-mochitest-e10s-devtools-chrome-1-bm126-tests1-windows-build9.txt.gz

I found:

14:12:44     INFO -  1467234764928	addons.xpi	WARN	Download of http://example.com/browser/devtools/client/debugger/test/mochitest//addon-webext-contentscript.xpi failed: signature is required but missing

I've got a signed version here:

https://people.mozilla.org/~amckay/addon-webext-contentscript.xpi 

Because apparently I'm bad at doing this, Andrew any chance you could review and check that in please?
please land on beta too if it works out
Blocks: 1186522
Pushed the update to inbound, aurora, and beta.
This won't solve the general problem but as an incremental improvement, is there any reason the devtools addon debugging tests can't just use temporary addon installation?  It looks like a non-trivial change, but it would save us some headaches in the long run.  I'm not familiar enough with the details of those tests to know if there's a good reason not to try.  Luca or Alexandre, any thoughts?
Flags: needinfo?(poirot.alex)
Flags: needinfo?(lgreco)
The test of the above xpi doesn't have anything that would prevent it to work with a temporary installation.

it looks like that by changing this mochitest helper:

- http://searchfox.org/mozilla-central/source/devtools/client/debugger/test/mochitest/head.js#114

we can learn if any of the test have any issue with the change.
Flags: needinfo?(lgreco)
The patch pushed on mozreview uses the approach described in Comment 4 to identify if any of the tests have issues with a temporary installation of the tested addon.

The good news is that most of the tests seems that can still make their job with temporary installed addons.

Unfortunately there are 2 tests that I'm not sure if they make sense with temporary installed addons:

- browser_dbg_addon-modules.js and browser_dbg_addon-modules-unpacked.js test that the sources urls include the path of the installed addon (and it seems that an XPI installed as a temporary addon is not unpacked)

Andrew, let me know if any of the devtools addon that we should cover is missing.

Alex, how it looks the strategy from the attached patch, any further thoughts related to this group of tests and the discussed change?
(In reply to Luca Greco [:rpl] from comment #6)
> - browser_dbg_addon-modules.js

This test seems to assume the js files are packed, so it should work?
Isn't it just that the modules jar: uri are slightly different?

> browser_dbg_addon-modules-unpacked.js

But this test does assume files are unpacked.
Can't we install temporary addon from a folder rather than a xpi file?
That way, files are going to be unpacked...
Flags: needinfo?(poirot.alex)
(In reply to Alexandre Poirot [:ochameau] from comment #7)
> (In reply to Luca Greco [:rpl] from comment #6)
> > - browser_dbg_addon-modules.js
> 
> This test seems to assume the js files are packed, so it should work?
> Isn't it just that the modules jar: uri are slightly different?
> 
> > browser_dbg_addon-modules-unpacked.js
> 
> But this test does assume files are unpacked.
> Can't we install temporary addon from a folder rather than a xpi file?
> That way, files are going to be unpacked...

sound great to me, and I think that the above approach will work. 

I already tried to convert the first one but I stopped to ask a feedback, in case I was missing something about the goal of these two tests.

Thanks for the feedback!
Comment on attachment 8768459 [details]
Bug 1283622 - add Devtools test addons as Temporary Addons.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62698/diff/1-2/
Applied changes based on suggestions from Comment 7:
added sources of the addon5 to the test assets and converted the remaining 2 tests to temporary addon.

Push to try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=180ff45e0a3f
Comment on attachment 8768459 [details]
Bug 1283622 - add Devtools test addons as Temporary Addons.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62698/diff/2-3/
Attachment #8768459 - Flags: review?(poirot.alex)
Removed a `dump` that wasn't intended to end in the patch pushed on mozreview.
Comment on attachment 8768459 [details]
Bug 1283622 - add Devtools test addons as Temporary Addons.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62698/diff/3-4/
I just learnt from the try build linked above that CHROME_URL was already defined as a const in a couple of other tests from the same dir, but it has the same meanings and it makes sense to share it through the shared head.js file.

Applied the changes needed to the patch and pushed the updated version on try:
- https://treeherder.mozilla.org/#/jobs?repo=try&revision=ad1da746d5eb
https://hg.mozilla.org/mozilla-central/rev/00588fae5ce6
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Comment on attachment 8768459 [details]
Bug 1283622 - add Devtools test addons as Temporary Addons.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62698/diff/4-5/
Comment on attachment 8768459 [details]
Bug 1283622 - add Devtools test addons as Temporary Addons.

https://reviewboard.mozilla.org/r/62698/#review59746

What happens with addon debugger title?

::: devtools/client/debugger/test/mochitest/browser_dbg_addon-modules-unpacked.js:18
(Diff revision 5)
> -    let addon = yield addAddon(ADDON_URL);
> +    let addon = yield addTemporaryAddon(ADDON_URL);
>      let tab1 = yield addTab("chrome://browser_dbg_addon5/content/test.xul");
>  
> -    let addonDebugger = yield initAddonDebugger(ADDON_URL);
> +    let addonDebugger = yield initAddonDebugger({ADDON_ID});
>  
> -    is(addonDebugger.title, `Developer Tools - Test unpacked add-on with JS Modules - ${ADDON_URL}`,
> +    is(addonDebugger.title, `Developer Tools - Test unpacked add-on with JS Modules - (null)`,

Hum. Is there something broken with temporary addon?
Why do we miss the addon url now?

::: devtools/client/debugger/test/mochitest/head.js:118
(Diff revision 5)
>    targetBrowser.removeTab(aTab);
>    return deferred.promise;
>  };
>  
> +function addTemporaryAddon(aChromeURL) {
> +  let aChromeURI = Services.io.newURI(aChromeURL, null, null);

Instead of forcing all test to construct the absolute URL, you may just pass a path by doing:

let uri = Services.io.newURI(aPath, null,
  Services.io.newURI(CHROME_URL));
  
Also, please use `a` prefix only for argument variables.
(aFile -> file, aChromeURI -> chromeURI)

::: devtools/client/debugger/test/mochitest/head.js:654
(Diff revision 5)
>    this._onConsoleAPICall = this._onConsoleAPICall.bind(this);
>    EventEmitter.decorate(this);
>  }
>  
>  AddonDebugger.prototype = {
> -  init: Task.async(function* (aUrl) {
> +  init: Task.async(function* ({ADDON_URL, ADDON_ID}) {

That looks very bad to have uppercased argument variable.
It looks like you are always passing an `id` now, so just replace aUrl by aId and pass just the id.
Then you can get rid of getAddonActorForUrl as well.
Attachment #8768459 - Flags: review?(poirot.alex)
(In reply to Alexandre Poirot [:ochameau] from comment #17)
> Comment on attachment 8768459 [details]
> Bug 1283622 - add Devtools test addons as Temporary Addons.
> 
> https://reviewboard.mozilla.org/r/62698/#review59746
> 
> What happens with addon debugger title?
> 
> :::
> devtools/client/debugger/test/mochitest/browser_dbg_addon-modules-unpacked.
> js:18
> (Diff revision 5)
> > -    let addon = yield addAddon(ADDON_URL);
> > +    let addon = yield addTemporaryAddon(ADDON_URL);
> >      let tab1 = yield addTab("chrome://browser_dbg_addon5/content/test.xul");
> >  
> > -    let addonDebugger = yield initAddonDebugger(ADDON_URL);
> > +    let addonDebugger = yield initAddonDebugger({ADDON_ID});
> >  
> > -    is(addonDebugger.title, `Developer Tools - Test unpacked add-on with JS Modules - ${ADDON_URL}`,
> > +    is(addonDebugger.title, `Developer Tools - Test unpacked add-on with JS Modules - (null)`,
> 
> Hum. Is there something broken with temporary addon?
> Why do we miss the addon url now?

ouch, I was sure that I mentioned it in the previous comment, but I have somehow missed to add the following in the previous comments:

It seems that the url is set only for addons that are installed from an url (e.g. installed from AMO) and currently it is not undefined when an addon is installed as a Temporary Addon.  

> ::: devtools/client/debugger/test/mochitest/head.js:118
> (Diff revision 5)
> >    targetBrowser.removeTab(aTab);
> >    return deferred.promise;
> >  };
> >  
> > +function addTemporaryAddon(aChromeURL) {
> > +  let aChromeURI = Services.io.newURI(aChromeURL, null, null);
> 
> Instead of forcing all test to construct the absolute URL, you may just pass
> a path by doing:
> 
> let uri = Services.io.newURI(aPath, null,
>   Services.io.newURI(CHROME_URL));

yeah, this is way better!

>   
> Also, please use `a` prefix only for argument variables.
> (aFile -> file, aChromeURI -> chromeURI)

sure, I forgot to change the names after the first experiment 

> 
> ::: devtools/client/debugger/test/mochitest/head.js:654
> (Diff revision 5)
> >    this._onConsoleAPICall = this._onConsoleAPICall.bind(this);
> >    EventEmitter.decorate(this);
> >  }
> >  
> >  AddonDebugger.prototype = {
> > -  init: Task.async(function* (aUrl) {
> > +  init: Task.async(function* ({ADDON_URL, ADDON_ID}) {
> 
> That looks very bad to have uppercased argument variable.
> It looks like you are always passing an `id` now, so just replace aUrl by
> aId and pass just the id.
> Then you can get rid of getAddonActorForUrl as well.

Agreed, I'm changing it
Comment on attachment 8768459 [details]
Bug 1283622 - add Devtools test addons as Temporary Addons.

Marked this attachment as obsolete, because we are moving this patch into the new Bug 1285289, opened as a follow up.
Attachment #8768459 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: