1128478 - sdk/panel's show/hide events not emitted if contentScriptWhen != 'ready'

Status: VERIFIED FIXED

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

Description

[Rob Wu [:robwu]]

Attachment: [e-mail]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:35.0) Gecko/20100101 Firefox/35.0
Build ID: 20150126171358

Steps to reproduce:

1. Create an addon with a panel without contentURL and with contentScriptWhen set to start.
2. Load the add-on in Firefox 36

let panel = require('sdk/panel').Panel({
  contentScriptWhen: 'start', // comment out this line, or set it to "ready" and the show event will be triggered.
});
panel.on('show', function() {
  panel.contentURL = 'data:text/plain,success!';
});

panel.show();


Actual results:

The show event is not triggered. The panel is empty.


Expected results:

 The show event should have been triggered. In the test case, you should see "success!" being displayed in the panel.

Comment 1

[Rob Wu [:robwu]]

Attachment: pull request

Comment 2

[Rob Wu [:robwu]]

Comment on attachment 8557887 [details] [review]
pull request

Hi Erik, could you review my patch?

Comment 3

[Erik Vold [:erikvold] (please needinfo? me)]

Comment on attachment 8557887 [details] [review]
pull request

Nice work!

Comment 4

[Erik Vold [:erikvold] (please needinfo? me)]

Comment on attachment 8557887 [details] [review]
pull request

I just mentioned one small issue with the test.

Comment 5

[[github robot]]

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

https://github.com/mozilla/addon-sdk/commit/6bcbca9ccd78eb557852eff3408e433c4f5e1c81
Bug 1128478 - sdk/panel's show/hide events not emitted if contentScriptWhen != 'ready'

https://github.com/mozilla/addon-sdk/commit/ad36ca4beea96572474cbe6a36430e9a5d9f281f
Merge pull request #1854 from Rob--W/panel-show-at-DOMContentLoaded

Bug 1128478 - sdk/panel's show/hide events not emitted if contentScriptWhen != 'ready' r=erikvold

Comment 6

[Rob Wu [:robwu]]

Erik, what can I do to ensure that this patch gets merged with the Addon SDK before Firefox 36 is promoted to stable? I already sent a mail to the mailing list, but haven't received a reply yet (https://groups.google.com/d/msg/mozilla-labs-jetpack/Gqz8UQkSohg/q_-pwuD75wEJ).

Comment 7

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Rob Wu from comment #6)
> Erik, what can I do to ensure that this patch gets merged with the Addon SDK
> before Firefox 36 is promoted to stable? I already sent a mail to the
> mailing list, but haven't received a reply yet
> (https://groups.google.com/d/msg/mozilla-labs-jetpack/Gqz8UQkSohg/q_-
> pwuD75wEJ).

After bug 1131923 is uplifted to m-c.

We'll need to make a patch for https://hg.mozilla.org/releases/mozilla-aurora which we can request an uplift for.

https://wiki.mozilla.org/Release_Management/Uplift_rules

Then we'd have to do the same for https://hg.mozilla.org/releases/mozilla-beta/

I'm going to do bug 1131923 this week, but I'm not sure I'll have time to follow through with uplifting this patch to beta, can you work on making this patch Rob? I can help you get it uplifted if so.

Comment 8

[Rob Wu [:robwu]]

Attachment: patch for aurora

I've generated a patch by recreating the folder structure of the affected files, using https://hg.mozilla.org/releases/mozilla-aurora/file/997fae1e1f88/ as base, and then manually applied the patch on top of it. Finally, I did git diff to generate a patch.

Is this patch in the right format?

Comment 9

[Lawrence Mandel [:lmandel] (use needinfo)]

It may be too late for beta as the last Beta is going to build today. ni Sylvestre about this one.

In order to uplift to Beta, can you please fill in the uplift approval questionnaire, which should auto-populate in the comments field when you select to submit an approval request?

Comment 10

[Sylvestre Ledru [:Sylvestre]]

Seems a bit late to take a patch that late in the beta cycle. Does this affect a lot of users?

Comment 11

[Lawrence Mandel [:lmandel] (use needinfo)]

ni Jorge in the hope that he can answer the question in comment 10.

Comment 12

[Jorge Villalobos [:jorgev] (he/him)]

I don't see any indications on this bug that this is a regression, so it looks like it can wait.

Comment 13

[Rob Wu [:robwu]]

This bug is a regression. It didn't happen in Firefox 35, and if the patch does not get uplifted, the bug will appear for the first time in Firefox 36.

This bug breaks addons, and came to my attention because another addon developer opened a ticket: https://github.com/Rob--W/browser-action-jplib/issues/18

Jorge: Given that your previous premise is wrong, can you re-evaluate the need for uplifting this patch?

Comment 14

[Sylvestre Ledru [:Sylvestre]]

Do we have an idea of which addons might be broken?

Comment 15

[Rob Wu [:robwu]]

I don't know. Is it possible to query the source code of all addons in AMO?
At the least, addons that use my library will be affected.

Listening to the show/hide events *and* having a content script that does not run at ready seems not that uncommon to me, but I haven't got any statistics to back up this claim.

Comment 16

[Jorge Villalobos [:jorgev] (he/him)]

(In reply to Sylvestre Ledru [:sylvestre] from comment #14)
> Do we have an idea of which addons might be broken?

Looking for require('browserAction') on the add-ons MXR returns 9 results. So, at least 9 add-ons are affected. The other possible affected code patterns would be too generic to yield useful results. I don't expect many popular add-ons to be affected by this, but it'll probably break quite a few small ones.

Given this new info, I'm in favor of uplifting, if possible.

Comment 17

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Rob Wu from comment #8)
> Created attachment 8565378 [details] [diff] [review]
> patch for aurora
> 
> I've generated a patch by recreating the folder structure of the affected
> files, using
> https://hg.mozilla.org/releases/mozilla-aurora/file/997fae1e1f88/ as base,
> and then manually applied the patch on top of it. Finally, I did git diff to
> generate a patch.
> 
> Is this patch in the right format?

Thanks for doing this!

It looks good to me, I've pushed it to the tbpl https://tbpl.mozilla.org/?tree=Try&rev=1b2ead88a73f if it passes which I expect then r+.

Comment 18

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Erik Vold [:erikvold] (please needinfo? me) from comment #17)
> (In reply to Rob Wu from comment #8)
> > Created attachment 8565378 [details] [diff] [review]
> > patch for aurora
> > 
> > I've generated a patch by recreating the folder structure of the affected
> > files, using
> > https://hg.mozilla.org/releases/mozilla-aurora/file/997fae1e1f88/ as base,
> > and then manually applied the patch on top of it. Finally, I did git diff to
> > generate a patch.
> > 
> > Is this patch in the right format?
> 
> Thanks for doing this!
> 
> It looks good to me, I've pushed it to the tbpl
> https://tbpl.mozilla.org/?tree=Try&rev=1b2ead88a73f if it passes which I
> expect then r+.

this is only failing on a known bug, so it lgtm!

Comment 19

[Erik Vold [:erikvold] (please needinfo? me)]

Comment on attachment 8565378 [details] [diff] [review]
patch for aurora

Approval Request Comment
[Feature/regressing bug #]: bug 1128478 fixes a regression in bug 1083391
[User impact if declined]: unknown, any add-on using a panel with contentScriptWhen = "start" will likely break.
[Describe test coverage new/current, TreeHerder]: I've tried the patch on aurora only so far, https://tbpl.mozilla.org/?tree=Try&rev=1b2ead88a73f&showall=1 the only failing tests are known issues resolved on m-c are were unaffected by the patch.
[Risks and why]: low risk I think because the amount of code affected is low, just the panel module, which already has the regression we are trying to fix.
[String/UUID change made/needed]: not sure what is being requested here..

Comment 20

[Sylvestre Ledru [:Sylvestre]]

Thanks Jorge for the investigation.

Comment 21

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8565378 [details] [diff] [review]
patch for aurora

[Triage Comment]
We merged beta => release.

Comment 22

[Ryan VanderMeulen (PTO, back 6-April)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b06ce7725bb7
https://hg.mozilla.org/releases/mozilla-beta/rev/c2a6bab25617

Comment 23

[Ryan VanderMeulen (PTO, back 6-April)]

https://hg.mozilla.org/releases/mozilla-release/rev/c2a6bab25617

The uplift is on Monday, so can we please make sure this lands on mozilla-central Very Soon?

Comment 24

[Rob Wu [:robwu]]

The patch has already landed on mozilla-central as a part of the Addon SDK uplift in bug 1131923.

Comment 25

[Bogdan Maris, Desktop Test Engineering]

Reproduced the initial issue using Firefox 36 beta 9, the issue does not reproduce anymore using Firefox 36.0 RC, latest Nightly and latest Aurora on Windows 7 64-bit, Ubuntu 14.04 32-bit and Mac OS X 10.9.5. Also did a sanity check on top 10 most popular add-ons, everything looks nice.