638292 - InstallTrigger is not defined when a new page is opened by clicking a link with target="_blank" or using window.open

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Stephen Donner [:stephend] Not actively reading bugmail]

STR:

1. Either through the Discovery Pane inside the Add-ons Manager, or a raw URL, like so, https://addons-next.allizom.org/en-US/firefox/discovery/pane/4.0/WINNT
2. Click on any Add-on
3. Now click on the "Learn More" button
4. Try to install the add-on from its "Add to Firefox" button

Actual Results:

Error: InstallTrigger is not defined
Source File: https://addons-next-cdn.allizom.org/media/js/common-min.js?build=1e96e1b
Line: 49

Screencast: http://screencast.com/t/Uv4hGtBdAhG

Comment 1

[Jeff Balogh (:jbalogh)]

When I reload after step 3 InstallTrigger comes back (according to completion in the web console).

Comment 2

[Dave Townsend [:mossop]]

Client bug, looks like DOMWindowCreated isn't firing for the new window for the page for some reason

Comment 3

[Dave Townsend [:mossop]]

Interestingly if I middle click the Learn More button a new tab is created and two DOMWindowCreated events fire, one for about:blank and then one for the page. If I click it normally two events fire again but both for about:blank.

What does the JS look like that is running when you click the link?

Comment 4

[Dave Townsend [:mossop]]

Regression since b7 at least, trying to narrow it down now.

Comment 5

[Dave Townsend [:mossop]]

Correction, this regressed somewhere between beta 6 and beta 7. The simplest way to check is to just load https://addons.mozilla.org/en-US/firefox/discovery/addon/read-it-later/?src=discovery-promo, click learn more and then install.

Comment 6

[Dave Townsend [:mossop]]

This is a regression from bug 582176.

Apparently there is no JS running here, the new tab opens because there is a <base target="_blank"> in the document.

Comment 7

[Dave Townsend [:mossop]]

Based on comment 3 it appears as if we get DOMWindowCreated for the new window but its document has about:blank loaded at the time rather than the real URL so the test here fails http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/content/extensions-content.js#235

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Hmm.. tricky. IIRC what we do under the hood is that we create one (inner) Window object which first contains a about:blank document, and later we replace the document inside that same window with the "real" document.

Since we don't create a new Window when that happens, we don't send out a content-document-global-created notification.

Comment 9

[Dave Townsend [:mossop]]

So I guess one easy way to fix this would just be to define InstallTrigger in the about:blank windows too, then when the document is replaces InstallTrigger remains?

I can't remember why we no longer define InstallTrigger in chrome and about pages, I know we used to in 3.6

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

That should work yes. Alternatively we could send out some other type of notification when replacing the about:blank document.

Though technically, I think web pages can use pushState to mutate about:blank pages into "real" urls, so it might make sense to install the InstallTrigger in about:blank pages too.

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

So does this mean that the Discover Pane is more or less completely borked?

Comment 12

[Asa Dotzler [:asa]]

I can't reproduce this. I opened add-ons manager, clicked the Add-on Compatibility Reporter promo, clicked Learn More which opened a new browser tab where I clicked the Add to Firefox button and it worked just fine.

Today's nightly on Win 7.

Comment 13

[Wil Clouser [:clouserw]]

(In reply to comment #12)
> I can't reproduce this. I opened add-ons manager, clicked the Add-on
> Compatibility Reporter promo, clicked Learn More which opened a new browser tab
> where I clicked the Add to Firefox button and it worked just fine.
> 
> Today's nightly on Win 7.

The URL link is not to a production server.  (Production is being updated to the same code at 1600 today though, fwiw)

Comment 14

[Dave Townsend [:mossop]]

I can reproduce this on production and my testcase is showing that this is a little more widespread than I thought.

Comment 15

[Dave Townsend [:mossop]]

(In reply to comment #11)
> So does this mean that the Discover Pane is more or less completely borked?

Not completely, it's kind of a special set of steps in that case but it seems easy enough for other sites to hit this so I agree we should consider blocking here.

Comment 16

[Edo Karabas]

WFM from Add-on manager, but fails from https://addons-next.allizom.org/en-US/firefox/discovery/pane/4.0/WINNT

Comment 17

[Mike Beltzner [:beltzner, not reading bugmail]]

Dave: feels like the only reason why we'd respin an RC for this bug is if it was a common patern we saw in the discovery pane. Is that the case?

Is this a recent regression?

Comment 18

[Asa Dotzler [:asa]]

(In reply to comment #15)
> (In reply to comment #11)
> > So does this mean that the Discover Pane is more or less completely borked?
> 
> Not completely, it's kind of a special set of steps in that case but it seems
> easy enough for other sites to hit this so I agree we should consider blocking
> here.

This is not happening commonly with the current AMO as far as I can tell.

I propose we not take the AMO change that tickles this Firefox bug, and count on AMO being the primary install site for add-ons, and relnote / techdoc for third parties on how to avoid the problem.

Comment 19

[Dave Townsend [:mossop]]

(In reply to comment #17)
> Dave: feels like the only reason why we'd respin an RC for this bug is if it
> was a common patern we saw in the discovery pane. Is that the case?

I would probably agree, however should we spin a new RC for other reasons I'd advocate to take the patch for this too, it will be quite safe, it's running through a full set of tests right now for me to verify it.

> Is this a recent regression?

No, it regressed in the nightlies just after beta 6 however the AMO discovery pane didn't have all the content it does right now so it is not surprising it wasn't spotted.

(In reply to comment #18)
> I propose we not take the AMO change that tickles this Firefox bug, and count
> on AMO being the primary install site for add-ons, and relnote / techdoc for
> third parties on how to avoid the problem.

I can reproduce this on the existing discovery pane so we'd actually have to remove functionality to hide the problem, functionality that I don't think we want to remove.

Comment 20

[Dave Townsend [:mossop]]

Actually there is a workaround possible. AMO's JS could test for the presence of InstallTrigger and if it doesn't exist revert to directly loading the XPI. I think it does this already in cases where InstallTrigger is disabled or JS is disabled but it is probably failing when InstallTrigger doesn't exist at all.

Comment 21

[Mike Beltzner [:beltzner, not reading bugmail]]

Marking as potential RC ridealong or .x fix; Dave, once your patch is through tryserver, please put it up for approval2.0?

Can we get a bug filed on the AMO workaround, too?

Comment 22

[Mike Beltzner [:beltzner, not reading bugmail]]

(actually, we can add fx-4-rc-ridealong once we have a patch!)

Comment 23

[Jeff Balogh (:jbalogh)]

(In reply to comment #21)
> Can we get a bug filed on the AMO workaround, too?

Bug 638781.

Comment 24

[Dave Townsend [:mossop]]

Attachment: patch rev 1

This fixes us to always define InstallTrigger for all types of windows which takes us back to the Firefox 3.6 behaviour. The testcase tries opening windows in three different ways and checking that InstallTrigger is defined there, the first two fail without this fix.

I'm spinning this through try a second time now and will pay extra attention to perf numbers before landing but I'm pretty sure this should be safe

Comment 25

[Robert Strong (they/them - no direct email)]

Comment on attachment 516945 [details] [diff] [review]
patch rev 1

The changes look safe enough as long as the perf numbers are good. My main concern is from looking at the comment where that check was added in bug 589598 comment #61

> Ok, I spent some more time investigating that issue. Looks like the patch
> wasn't identifying chrome windows properly - the problem is not just actual
> ChromeWindows, but also normal windows showing |chrome://| or |about:| pages.
> Checking for those directly also simplifies the error checking in the patch.

It looks like the change this patch removes was related to bug 589598 comment #60
> >+    // For unknown reasons this raises an error sometimes. This is not
> >+    // because there already is a getter there. It is also not because
> >+    // we mistakenly try to place one on chrome. In any case it does not
> >+    // prevent this from working.
> >+    try {
> >+      window.wrappedJSObject.__defineGetter__("InstallTrigger", this.getter);
> >+    }
> >+    catch(e) {
> >+      dump("Ignored InstallTrigger setup error\n");
> >+    }
> >+  },

but this code wasn't added back with that removal. I'm worried that the following will randomly fail for unknown reasons since it appears that was happening in bug 589598.
window.wrappedJSObject.__defineGetter__("InstallTrigger" etc.

minusing due to wanting the above answered

Comment 26

[Dave Townsend [:mossop]]

Alon, do you have more info about what was going on there?

Worst case if that throws here then InstallTrigger just isn't defined for that window.

Comment 27

[u279076]

NOTE: This also happens through https://addons.mozilla.org/en-US/firefox/compatibility/report/4.0 if I click a link to an add-on then the Add to Firefox button on the page.  I need to reload the page to get the Add to Firefox button to work.

Comment 28

[Robert Strong (they/them - no direct email)]

Comment on attachment 516945 [details] [diff] [review]
patch rev 1

(In reply to comment #26)
> Alon, do you have more info about what was going on there?
> 
> Worst case if that throws here then InstallTrigger just isn't defined for that
> window.

It would be good to get more info as to what was going on from Alon though I think this is safe as is pending the perf results.

Comment 29

[Alon Zakai (:azakai)]

We got an error back then when trying to place InstallTriggers on chrome windows. It was a security error of some form (I seem to recall a bug on it, but don't see it right now).

The error came up in the normal InstallTrigger tests, so if you aren't seeing it on try, then I would assume that it is no longer an issue.

Comment 30

[Dave Townsend [:mossop]]

Comment on attachment 516945 [details] [diff] [review]
patch rev 1

Perf results compared against the 10 most recent changesets on m-c are here: http://bit.ly/gNLVu4

One or two red marks but all small and mostly in areas where I wouldn't expect this patch to have any effect. The one oddity is a 1.38% increase in Ts on OSX however that is countered by all the other platforms decreasing in Ts and I did a second run (http://bit.ly/fFf0Gm) which showed only a .23% increase (although a 1.9% increase in Linux64 this time).

I'm judging those numbers to be effectively clear of any real change in performance.

I added the try...catch locally and ran full tests and checked over the log files and saw no indication of InstallTrigger being failed to be defined.

Comment 31

[Asa Dotzler [:asa]]

Comment on attachment 516945 [details] [diff] [review]
patch rev 1

If we can fix on AMO for Firefox 4 (or not regress AMO) then we should do that. Not going to block on the code changes.

Comment 32

[Wil Clouser [:clouserw]]

(In reply to comment #31)
> Comment on attachment 516945 [details] [diff] [review]
> patch rev 1
> 
> If we can fix on AMO for Firefox 4 (or not regress AMO) then we should do that.
> Not going to block on the code changes.

AMO's bug is already closed.  You can verify it on addons.allizom.org

Comment 33

[Dave Townsend [:mossop]]

Landed on trunk: http://hg.mozilla.org/mozilla-central/rev/ebb87a57ed5b

Comment 34

[Gene]

Verified: Mozilla/5.0 (Windows NT 6.0; rv:2.0.1) Gecko/20100101 Firefox/4.0.1

Comment 35

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 02/17 - 02/21)]

(In reply to comment #34)
> Verified: Mozilla/5.0 (Windows NT 6.0; rv:2.0.1) Gecko/20100101 Firefox/4.0.1

This fix cannot be verified with Firefox 4.0.1 because it never landed on the mozilla-2.0 branch.

Comment 36

[u279076]

WFM using Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:6.0a1) Gecko/20110506 Firefox/6.0a1

Can this be marked VERIFIED?

Comment 37

[Tobias (:Tobbi) Markus]

Verified FIXED using latest nightly on Windows.

Comment 38

[Neil Deakin]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f419731a1af