1358314 - 'Open Link in New Tab' does not work after unloading document

Reporter

Description

•

8 years ago

0. Disable e10s 1. Click a link which points to a slow server (bugzilla.mozilla.org is a good example) 2. Before the document unloads, right click another link 3. Wait until the new document loads 4. Click 'Open Link in New Tab' in the context menu Expected result: a new tab loads the url of the 2nd link Result: no new tab, an error is thrown. TypeError: window is null (resource://gre/modules/WebNavigationFrames.jsm:59:1) getFrameId (resource://gre/modules/WebNavigationFrames.jsm:59:1) _openLinkInParameters (chrome://browser/content/nsContextMenu.js:1036:35) openLinkInTab (chrome://browser/content/nsContextMenu.js:1085:37) oncommand (chrome://browser/content/browser.xul:1:1) If you follow the same steps, but now inside an iframe, the error is TypeError: can't access dead object (chrome://browser/content/nsContextMenu.js) _openLinkInParameters (chrome://browser/content/nsContextMenu.js:1036:7) openLinkInTab (chrome://browser/content/nsContextMenu.js:1085:37) oncommand (chrome://browser/content/browser.xul:1:1) The problem is if (!this.isRemote) { params.frameOuterWindowID = WebNavigationFrames.getFrameId(this.target.ownerGlobal); } In the iframe case, this.target is a DeadObject, so this.target.ownerGlobal throws. In the non-iframe case, this.target.ownerGlobal is null, so window.parent === window throws in getFrameId. This worked correctly before bug 1190687. This is a regression (bug 1025568), so please add automated tests to ensure this doesn't happen a third time.

Flags: needinfo?(lgreco)

Cosmin Muntean [:cmuntean], Ecosystem QA (PTO until September 8th)

Updated

•

8 years ago

status-firefox53: --- → unaffected

status-firefox54: --- → affected

status-firefox55: --- → affected

Luca Greco [:rpl] [:luca] [:lgreco]

Assignee

Updated

•

8 years ago

Assignee: nobody → lgreco

Status: NEW → ASSIGNED

Comment hidden (mozreview-request)

Luca Greco [:rpl] [:luca] [:lgreco]

Assignee

Updated

•

8 years ago

Attachment #8860988 - Flags: review?(kmaglione+bmo)

Comment hidden (mozreview-request)

Kris Maglione [:kmag]

Comment 3

•

8 years ago

mozreview-review

Comment on attachment 8860988 [details] Bug 1358314 - Fix 'Open Link in New Tab' context menu item after tab navigated in non-e10s mode. https://reviewboard.mozilla.org/r/133018/#review138946 r=me with the sjs script converted to a static HTML page. ::: browser/base/content/test/tabs/test_bug1358314.sjs:17 (Diff revision 2) > + </body> > + </html>`; > + response.setStatusLine(request.httpVersion, "200", "OK"); > + response.setHeader("Content-Type", "text/html", false); > + response.setHeader("Content-Length", page.length + "", false); > + response.write(page); There's no need to use an sjs script for this.

Attachment #8860988 - Flags: review?(kmaglione+bmo) → review+

Comment hidden (mozreview-request)

Luca Greco [:rpl] [:luca] [:lgreco]

Assignee

Comment 5

•

8 years ago

mozreview-review-reply

Comment on attachment 8860988 [details] Bug 1358314 - Fix 'Open Link in New Tab' context menu item after tab navigated in non-e10s mode. https://reviewboard.mozilla.org/r/133018/#review138946 > There's no need to use an sjs script for this. yep, sure! (I was initially going to reproduce the issue as described, with the slow loading page, but then I noticed that the issue can be more easily reproduced without introducing any latency in the page loading and the .sjs was returning basically a static HTML page). patch updated by turning the sjs script into a static html page. Thanks Kris.

Luca Greco [:rpl] [:luca] [:lgreco]

Assignee

Updated

•

8 years ago

Flags: needinfo?(lgreco)

Keywords: checkin-needed

Pulsebot

Comment 6

•

8 years ago

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/78a53da8a8c1 Fix 'Open Link in New Tab' context menu item after tab navigated in non-e10s mode. r=kmag

Keywords: checkin-needed

Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)

Comment 7

•

8 years ago

Flags: needinfo?(lgreco)

Comment hidden (mozreview-request)

Luca Greco [:rpl] [:luca] [:lgreco]

Assignee

Comment 10

•

8 years ago

As briefly discussed over IRC, I suspected that the leak was related to the test case, and after reproducing it locally, I looked into the test to identify which part of the test could generate the leak and applied the following changes: - added a missing yield on the two BrowserTestUtils.removeTab calls, to ensure that the two tabs have been completely closed before exiting the test - added a call to contextMenu.hidePopup, to ensure that the context menu has been closed before exiting the test (and I think that this was the one that was generating the leak) the updated patch pushed to try: - https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0c3754b4ab285c4dc352245c317c6278f67f5c8

Flags: needinfo?(lgreco)

Keywords: checkin-needed

Luca Greco [:rpl] [:luca] [:lgreco]

Assignee

Comment 11

•

8 years ago

Actually, the above link to treeherder is related to the same updated patch adapted to beta and pushed to try to test it. The treeherder link to the try build with the updated patch applied on mozilla-central is the following: - https://treeherder.mozilla.org/#/jobs?repo=try&revision=f798655d60a3f3b067b8ca366cd0f96eced539b0

Pulsebot

Comment 12

•

8 years ago

Pushed by ihsiao@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/fea9c89ca6a5 Fix 'Open Link in New Tab' context menu item after tab navigated in non-e10s mode. r=kmag

Keywords: checkin-needed

Carsten Book [:Tomcat]

Comment 13

•

8 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/fea9c89ca6a5

Status: ASSIGNED → RESOLVED

Closed: 8 years ago

status-firefox55: affected → fixed

Resolution: --- → FIXED

Target Milestone: --- → Firefox 55

Gerry Chang [:gchang]

Comment 14

•

8 years ago

Hi Cosmin, Can you help check if the issue is fixed in the latest nightly?

Flags: qe-verify+

Flags: needinfo?(cosmin.muntean)

Cosmin Muntean [:cmuntean], Ecosystem QA (PTO until September 8th)

Comment 15

•

8 years ago

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:55.0) Gecko/20100101 Firefox/55.0 Firefox 55.0a1, Build ID: 20170507030205 Hi Garry, I have tested this issue on latest Nightly (55.0a1, Build ID: 20170507030205) build, and the issue is no longer reproducible. I have tested this on Windows, Linux and Mac OS. I can confirm that the issue was reproducible on older Nightly builds (eg: 55.0a1, 2017-05-04, Build ID: 20170504030320).

status-firefox55: fixed → verified

Flags: needinfo?(cosmin.muntean)

Ryan VanderMeulen [:RyanVM]

Comment 16

•

8 years ago

Please request Beta approval on this when you get a chance.

Status: RESOLVED → VERIFIED

status-firefox-esr52: --- → unaffected

Flags: needinfo?(lgreco)

Luca Greco [:rpl] [:luca] [:lgreco]

Assignee

Comment 17

•

8 years ago

Attached patch rebase_on_beta-Bug_1358314___Fix__Open_Link_in_New_Tab__context_menu_item_after_tab_navigated_in_non_e10s_mode_.patch (deleted) — Details — Splinter Review

This new attachment is the same patch submitted to mozreview and landed on mozilla-central, adapted to be applied on top of beta. Follows the approval-mozilla-beta request comment. Approval Request Comment [Feature/Bug causing the regression]: Bug 1190687 (Implement webNavigation.onCreatedNavigationTarget) introduced a regression on the "Open Link in ..." context menu actions when Firefox is running in non-e10s mode. [User impact if declined]: When Firefox is running in non-e10s mode and a user opens a context menu on a link in a webpage, if the webpage that contains the selected link is navigated while the context menu is still open, then clicking the "Open Link in ..." actions doesn't open the selected url in a new tab (or window) as the user expects (and an exception is raised in the browser console) [Is this code covered by automated tests?]: Yes, the patch adds an additional test case that covers the regressed scenario. [Has the fix been verified in Nightly?]: Yes, the fix has been landed and verified in Nightly. [Needs manual test from QE? If yes, steps to reproduce]: Yes, it would be nice to also test manually that the issue has been fixed on beta using the STR from Comment 0. [List of other uplifts needed for the feature/fix]: None [Is the change risky?]: The change has low risks. [Why is the change risky/not risky?]: The fix is pretty small (it is a single line change) and its implementation is simpler and less risky of the one currently implemented in beta, and new automated tests has been added to the patch (actually, most of the patch is the new test case) . [String changes made/needed]: None

Flags: needinfo?(lgreco)

Attachment #8865840 - Flags: approval-mozilla-beta?

Gerry Chang [:gchang]

Comment 18

•

8 years ago

Comment on attachment 8865840 [details] [diff] [review] rebase_on_beta-Bug_1358314___Fix__Open_Link_in_New_Tab__context_menu_item_after_tab_navigated_in_non_e10s_mode_.patch Fix a regression and was verified. Beta54+. Should be in 54 beta 7.

Attachment #8865840 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Luca Greco [:rpl] [:luca] [:lgreco]

Assignee

Comment 19

•

8 years ago

Attachment 8865840 [details] [diff] (patch rebased for beta) pushed to try (green, besides a couple of known intermittents): https://treeherder.mozilla.org/#/jobs?repo=try&revision=95fc6a63507b7e24bd57f5afd279a452182788fc

Ryan VanderMeulen [:RyanVM]

Comment 20

•

8 years ago

bugherder uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/ecca51aa18de

status-firefox54: affected → fixed

Flags: in-testsuite+

Timea Zsoldos [:zstimi/tzsoldos], Desktop QA

Comment 21

•

8 years ago

I have reproduced this issue using Firefox 55.0a1 (2017.05.04) on Win 8.1 x64, build ID:20170504030320. I can confirm this issue is fixed, I verified using Firefox 54.0b7 on Win 8.1 x64, Mac 10.11 and Ubuntu 14.04 x64.

status-firefox54: fixed → verified

Flags: qe-verify+

Bug 1358314 - Fix 'Open Link in New Tab' context menu item after tab navigated in non-e10s mode. 8 years ago Luca Greco [:rpl] [:luca] [:lgreco] (deleted), text/x-review-board-request	kmag : review+	Details
rebase_on_beta-Bug_1358314___Fix__Open_Link_in_New_Tab__context_menu_item_after_tab_navigated_in_non_e10s_mode_.patch 8 years ago Luca Greco [:rpl] [:luca] [:lgreco] (deleted), patch	gchang : approval-mozilla-beta+	Details \| Diff \| Splinter Review