Closed Bug 1128478 Opened 5 years ago Closed 5 years ago

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

Categories

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

defect

Tracking

(firefox35 unaffected, firefox36+ verified, firefox37+ verified, firefox38+ verified)

VERIFIED FIXED
mozilla38
Tracking Status
firefox35 --- unaffected
firefox36 + verified
firefox37 + verified
firefox38 + verified

People

(Reporter: robwu, Assigned: robwu)

References

Details

Attachments

(3 files)

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.
Component: Untriaged → General
Product: Firefox → Add-on SDK
Version: 36 Branch → unspecified
Attached file pull request
Comment on attachment 8557887 [details] [review]
pull request

Hi Erik, could you review my patch?
Attachment #8557887 - Flags: review?(evold)
Assignee: nobody → rob
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment on attachment 8557887 [details] [review]
pull request

Nice work!
Attachment #8557887 - Flags: review?(evold) → review+
Comment on attachment 8557887 [details] [review]
pull request

I just mentioned one small issue with the test.
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
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).
Flags: needinfo?(evold)
Flags: needinfo?(evold)
OS: Linux → All
Hardware: x86_64 → All
(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.
Flags: needinfo?(rob)
Attached patch patch for auroraSplinter Review
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?
Flags: needinfo?(rob)
Attachment #8565378 - Flags: review?(evold)
Attachment #8565378 - Flags: approval-mozilla-aurora?
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?
Flags: needinfo?(sledru)
Seems a bit late to take a patch that late in the beta cycle. Does this affect a lot of users?
Flags: needinfo?(sledru)
ni Jorge in the hope that he can answer the question in comment 10.
Flags: needinfo?(jorge)
I don't see any indications on this bug that this is a regression, so it looks like it can wait.
Flags: needinfo?(jorge)
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?
Flags: needinfo?(jorge)
Do we have an idea of which addons might be broken?
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.
(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.
Flags: needinfo?(jorge)
(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+.
(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!
Attachment #8565378 - Flags: review?(evold) → review+
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..
Attachment #8565378 - Flags: approval-mozilla-beta?
Priority: -- → P1
Attachment #8565378 - Flags: approval-mozilla-beta?
Attachment #8565378 - Flags: approval-mozilla-beta+
Attachment #8565378 - Flags: approval-mozilla-aurora?
Attachment #8565378 - Flags: approval-mozilla-aurora+
Thanks Jorge for the investigation.
Comment on attachment 8565378 [details] [diff] [review]
patch for aurora

[Triage Comment]
We merged beta => release.
Attachment #8565378 - Flags: approval-mozilla-beta+ → approval-mozilla-release+
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?
Flags: needinfo?(rob)
Flags: needinfo?(evold)
The patch has already landed on mozilla-central as a part of the Addon SDK uplift in bug 1131923.
Flags: needinfo?(rob)
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: needinfo?(evold)
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
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.
You need to log in before you can comment on or make changes to this bug.