Closed Bug 638292 Opened 9 years ago Closed 9 years ago

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

Categories

(Toolkit :: Add-ons Manager, defect, blocker)

defect
Not set
blocker

Tracking

()

VERIFIED FIXED
mozilla5
Tracking Status
blocking2.0 --- .x+

People

(Reporter: stephend, Assigned: mossop)

References

()

Details

(Keywords: regression, Whiteboard: [bugday-2011-05-06])

Attachments

(1 file)

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
When I reload after step 3 InstallTrigger comes back (according to completion in the web console).
Client bug, looks like DOMWindowCreated isn't firing for the new window for the page for some reason
Component: Public Pages → Add-ons Manager
Product: addons.mozilla.org → Toolkit
QA Contact: web-ui → add-ons.manager
Target Milestone: 5.12.12 → ---
Version: unspecified → Trunk
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?
Regression since b7 at least, trying to narrow it down now.
Keywords: regression
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.
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.
Blocks: 582176
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
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.
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
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.
So does this mean that the Discover Pane is more or less completely borked?
blocking2.0: --- → ?
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.
(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)
I can reproduce this on production and my testcase is showing that this is a little more widespread than I thought.
Summary: InstallTrigger is not defined when coming from the "Learn More" link in the Discovery Pane → InstallTrigger is not defined when a new page is opened by clicking a link with target="_blank" or using window.open
(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.
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?
(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.
(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.
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.
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?
blocking2.0: ? → .x+
Whiteboard: [fx4-rc-ridealong]
Whiteboard: [fx4-rc-ridealong] → [fx4-rc-ridealong][addons-testday]
(actually, we can add fx-4-rc-ridealong once we have a patch!)
Whiteboard: [fx4-rc-ridealong][addons-testday] → [addons-testday]
Depends on: 638781
(In reply to comment #21)
> Can we get a bug filed on the AMO workaround, too?

Bug 638781.
Attached patch patch rev 1Splinter Review
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
Assignee: nobody → dtownsend
Status: NEW → ASSIGNED
Attachment #516945 - Flags: review?(robert.bugzilla)
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
Attachment #516945 - Flags: review?(robert.bugzilla) → review-
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.
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 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.
Attachment #516945 - Flags: review- → review+
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 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.
Attachment #516945 - Flags: approval2.0?
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.
Attachment #516945 - Flags: approval2.0? → approval2.0-
(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
Whiteboard: [addons-testday] → [can land fx5]
Landed on trunk: http://hg.mozilla.org/mozilla-central/rev/ebb87a57ed5b
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Flags: in-litmus-
Resolution: --- → FIXED
Whiteboard: [can land fx5]
Verified: Mozilla/5.0 (Windows NT 6.0; rv:2.0.1) Gecko/20100101 Firefox/4.0.1
Status: RESOLVED → VERIFIED
(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.
Status: VERIFIED → RESOLVED
Closed: 9 years ago9 years ago
Target Milestone: --- → mozilla5
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?
Verified FIXED using latest nightly on Windows.
Status: RESOLVED → VERIFIED
Whiteboard: [bugday-2011-05-06]
You need to log in before you can comment on or make changes to this bug.