Closed
Bug 1358314
Opened 8 years ago
Closed 8 years ago
'Open Link in New Tab' does not work after unloading document
Categories
(Firefox :: Menus, defect)
Tracking
()
VERIFIED
FIXED
Firefox 55
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox53 | --- | unaffected |
firefox54 | --- | verified |
firefox55 | --- | verified |
People
(Reporter: Oriol, Assigned: rpl)
References
Details
(Keywords: regression)
Attachments
(2 files)
59 bytes,
text/x-review-board-request
|
kmag
:
review+
|
Details |
4.37 KB,
patch
|
gchang
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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)
Updated•8 years ago
|
status-firefox53:
--- → unaffected
status-firefox54:
--- → affected
status-firefox55:
--- → affected
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → lgreco
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8860988 -
Flags: review?(kmaglione+bmo)
Comment hidden (mozreview-request) |
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) |
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.
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(lgreco)
Keywords: checkin-needed
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
![]() |
||
Comment 7•8 years ago
|
||
Backed out for intermittent, different leaks on Windows 7 VM debug:
https://hg.mozilla.org/integration/autoland/rev/4724794816df1f097f3d2bfc3f7e1561d39abfc3
Push with leaks: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=78a53da8a8c1e5fad3d544e01fae40a23571075c&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Log with leaks: https://treeherder.mozilla.org/logviewer.html#?job_id=96645387&repo=autoland
09:44:00 INFO - Stopping ssltunnel
09:44:00 INFO - TEST-INFO | leakcheck | default process: leak threshold set at 0 bytes
09:44:00 INFO - TEST-INFO | leakcheck | plugin process: leak threshold set at 0 bytes
09:44:00 INFO - TEST-INFO | leakcheck | tab process: leak threshold set at 10000 bytes
09:44:00 INFO - TEST-INFO | leakcheck | geckomediaplugin process: leak threshold set at 20000 bytes
09:44:00 INFO - TEST-INFO | leakcheck | gpu process: leak threshold set at 0 bytes
09:44:00 INFO -
09:44:00 INFO - == BloatView: ALL (cumulative) LEAK AND BLOAT STATISTICS, default process 860
09:44:00 INFO -
09:44:00 INFO - |<----------------Class--------------->|<-----Bytes------>|<----Objects---->|
09:44:00 INFO - | | Per-Inst Leaked| Total Rem|
09:44:00 INFO - 0 |TOTAL | 18 3672|10467062 74|
09:44:00 INFO - 2 |APZEventState | 112 112| 3 1|
09:44:00 INFO - 7 |ActiveElementManager | 24 24| 3 1|
09:44:00 INFO - 38 |BackstagePass | 52 104| 330 2|
09:44:00 INFO - 127 |CondVar | 36 72| 661 2|
09:44:00 INFO - 312 |IAPZCTreeManager | 8 8| 3 1|
09:44:00 INFO - 330 |IdlePeriod | 12 12| 63 1|
09:44:00 INFO - 353 |InputQueue | 36 36| 3 1|
09:44:00 INFO - 410 |Mutex | 44 176| 4507 4|
09:44:00 INFO - 637 |TSFStaticSink | 56 56| 1 1|
09:44:00 INFO - 638 |TSFTextStore | 212 212| 3 1|
09:44:00 INFO - 647 |TextEventDispatcher | 76 76| 1 1|
09:44:00 INFO - 733 |WinTextEventDispatcherListener | 16 16| 1 1|
09:44:00 INFO - 762 |XPCNativeInterface | 28 28| 2881 1|
09:44:00 INFO - 763 |XPCNativeMember | 8 8| 64661 1|
09:44:00 INFO - 764 |XPCNativeSet | 16 16| 1444 1|
09:44:00 INFO - 766 |XPCWrappedNative | 48 96| 28788 2|
09:44:00 INFO - 767 |XPCWrappedNativeProto | 20 40| 7643 2|
09:44:00 INFO - 770 |XPCWrappedNativeTearOff | 16 32| 37932 2|
09:44:00 INFO - 776 |ZoomConstraints | 12 180| 367 15|
09:44:00 INFO - 866 |nsBaseWidget | 272 272| 7 1|
09:44:00 INFO - 1099 |nsIdleService | 88 88| 1 1|
09:44:00 INFO - 1100 |nsIdleServiceDaily | 64 64| 1 1|
09:44:00 INFO - 1101 |nsIdleServiceWin | 88 88| 1 1|
09:44:00 INFO - 1128 |nsJSPrincipals | 16 16| 3787 1|
09:44:00 INFO - 1283 |nsStringBuffer | 8 16| 360311 2|
09:44:00 INFO - 1331 |nsTArray_base | 4 48| 2212370 12|
09:44:00 INFO - 1340 |nsThread | 248 248| 62 1|
09:44:00 INFO - 1344 |nsTimer | 16 32| 2099 2|
09:44:00 INFO - 1345 |nsTimerImpl | 136 272| 2099 2|
09:44:00 INFO - 1378 |nsWeakReference | 20 40| 769 2|
09:44:00 INFO - 1383 |nsWindow | 920 920| 7 1|
09:44:00 INFO - 1426 |nsXPCWrappedJS | 60 180| 4362 3|
09:44:00 INFO - 1427 |nsXPCWrappedJSClass | 44 44| 566 1|
09:44:00 INFO - 1451 |xptiInterfaceInfo | 20 40| 1384 2|
09:44:00 INFO -
09:44:00 INFO - nsTraceRefcnt::DumpStatistics: 1452 entries
09:44:00 INFO - TEST-INFO | leakcheck | default process: leaked 1 APZEventState
09:44:00 INFO - TEST-INFO | leakcheck | default process: leaked 1 ActiveElementManager
09:44:00 INFO - TEST-INFO | leakcheck | default process: leaked 2 BackstagePass
09:44:00 INFO - TEST-INFO | leakcheck | default process: leaked 2 CondVar
09:44:00 INFO - TEST-INFO | leakcheck | default process: leaked 1 IAPZCTreeManager
09:44:00 INFO - TEST-INFO | leakcheck | default process: leaked 1 IdlePeriod
09:44:00 INFO - TEST-INFO | leakcheck | default process: leaked 1 InputQueue
09:44:00 INFO - TEST-INFO | leakcheck | default process: leaked 4 Mutex
09:44:00 INFO - TEST-INFO | leakcheck | default process: leaked 1 TSFStaticSink
09:44:00 INFO - TEST-INFO | leakcheck | default process: leaked 1 TSFTextStore
09:44:00 INFO - TEST-INFO | leakcheck | default process: leaked 1 TextEventDispatcher
09:44:00 INFO - TEST-INFO | leakcheck | default process: leaked 1 WinTextEventDispatcherListener
09:44:00 INFO - TEST-INFO | leakcheck | default process: leaked 1 XPCNativeInterface
09:44:00 INFO - TEST-INFO | leakcheck | default process: leaked 1 XPCNativeMember
09:44:00 INFO - TEST-INFO | leakcheck | default process: leaked 1 XPCNativeSet
09:44:00 INFO - TEST-INFO | leakcheck | default process: leaked 2 XPCWrappedNative
09:44:00 INFO - TEST-INFO | leakcheck | default process: leaked 2 XPCWrappedNativeProto
09:44:00 INFO - TEST-INFO | leakcheck | default process: leaked 2 XPCWrappedNativeTearOff
09:44:00 INFO - TEST-INFO | leakcheck | default process: leaked 15 ZoomConstraints
09:44:00 INFO - TEST-INFO | leakcheck | default process: leaked 1 nsBaseWidget
09:44:00 INFO - TEST-INFO | leakcheck | default process: leaked 1 nsIdleService
09:44:00 INFO - TEST-INFO | leakcheck | default process: leaked 1 nsIdleServiceDaily
09:44:00 INFO - TEST-INFO | leakcheck | default process: leaked 1 nsIdleServiceWin
09:44:00 INFO - TEST-INFO | leakcheck | default process: leaked 1 nsJSPrincipals
09:44:00 INFO - TEST-INFO | leakcheck | default process: leaked 2 nsStringBuffer
09:44:00 INFO - TEST-INFO | leakcheck | default process: leaked 12 nsTArray_base
09:44:00 INFO - TEST-INFO | leakcheck | default process: leaked 1 nsThread
09:44:00 INFO - TEST-INFO | leakcheck | default process: leaked 2 nsTimer
09:44:00 INFO - TEST-INFO | leakcheck | default process: leaked 2 nsTimerImpl
09:44:00 INFO - TEST-INFO | leakcheck | default process: leaked 2 nsWeakReference
09:44:00 INFO - TEST-INFO | leakcheck | default process: leaked 1 nsWindow
09:44:00 INFO - TEST-INFO | leakcheck | default process: leaked 3 nsXPCWrappedJS
09:44:00 INFO - TEST-INFO | leakcheck | default process: leaked 1 nsXPCWrappedJSClass
09:44:00 INFO - TEST-INFO | leakcheck | default process: leaked 2 xptiInterfaceInfo
09:44:00 ERROR - TEST-UNEXPECTED-FAIL | leakcheck | default process: 3672 bytes leaked (APZEventState, ActiveElementManager, BackstagePass, CondVar, IAPZCTreeManager, ...)
09:44:00 INFO -
09:44:00 INFO - == BloatView: ALL (cumulative) LEAK AND BLOAT STATISTICS, tab process 1328
09:44:00 INFO -
09:44:00 INFO - |<----------------Class--------------->|<-----Bytes------>|<----Objects---->|
09:44:00 INFO - | | Per-Inst Leaked| Total Rem|
09:44:00 INFO - 0 |TOTAL | 20 0| 1031278 0|
09:44:00 INFO -
09:44:00 INFO - nsTraceRefcnt::DumpStatistics: 772 entries
09:44:00 INFO -
09:44:00 INFO - TEST-PASS | leakcheck | tab process: no leaks detected!
09:44:00 INFO -
09:44:00 INFO - == BloatView: ALL (cumulative) LEAK AND BLOAT STATISTICS, tab process 1584
09:44:00 INFO -
09:44:00 INFO - |<----------------Class--------------->|<-----Bytes------>|<----Objects---->|
09:44:00 INFO - | | Per-Inst Leaked| Total Rem|
09:44:00 INFO - 0 |TOTAL | 40 0| 59155 0|
09:44:00 INFO -
09:44:00 INFO - nsTraceRefcnt::DumpStatistics: 739 entries
09:44:00 INFO -
09:44:00 INFO - TEST-PASS | leakcheck | tab process: no leaks detected!
09:44:00 INFO -
09:44:00 INFO - == BloatView: ALL (cumulative) LEAK AND BLOAT STATISTICS, tab process 2756
09:44:00 INFO -
09:44:00 INFO - |<----------------Class--------------->|<-----Bytes------>|<----Objects---->|
09:44:00 INFO - | | Per-Inst Leaked| Total Rem|
09:44:00 INFO - 0 |TOTAL | 19 0| 949325 0|
09:44:00 INFO -
09:44:00 INFO - nsTraceRefcnt::DumpStatistics: 739 entries
09:44:00 INFO -
09:44:00 INFO - TEST-PASS | leakcheck | tab process: no leaks detected!
09:44:00 INFO -
09:44:00 INFO - == BloatView: ALL (cumulative) LEAK AND BLOAT STATISTICS, tab process 3548
09:44:00 INFO -
09:44:00 INFO - |<----------------Class--------------->|<-----Bytes------>|<----Objects---->|
09:44:00 INFO - | | Per-Inst Leaked| Total Rem|
09:44:00 INFO - 0 |TOTAL | 23 0| 217043 0|
09:44:00 INFO -
09:44:00 INFO - nsTraceRefcnt::DumpStatistics: 720 entries
09:44:00 INFO -
09:44:00 INFO - TEST-PASS | leakcheck | tab process: no leaks detected!
09:44:00 INFO -
09:44:00 INFO - == BloatView: ALL (cumulative) LEAK AND BLOAT STATISTICS, tab process 4992
09:44:00 INFO -
09:44:00 INFO - |<----------------Class--------------->|<-----Bytes------>|<----Objects---->|
09:44:00 INFO - | | Per-Inst Leaked| Total Rem|
09:44:00 INFO - 0 |TOTAL | 19 0| 1060040 0|
09:44:00 INFO -
09:44:00 INFO - nsTraceRefcnt::DumpStatistics: 764 entries
09:44:00 INFO -
09:44:00 INFO - TEST-PASS | leakcheck | tab process: no leaks detected!
09:44:00 INFO -
09:44:00 INFO - == BloatView: ALL (cumulative) LEAK AND BLOAT STATISTICS, tab process 5636
09:44:00 INFO -
09:44:00 INFO - |<----------------Class--------------->|<-----Bytes------>|<----Objects---->|
09:44:00 INFO - | | Per-Inst Leaked| Total Rem|
09:44:00 INFO - 0 |TOTAL | 22 0| 227950 0|
09:44:00 INFO -
09:44:00 INFO - nsTraceRefcnt::DumpStatistics: 725 entries
09:44:00 INFO -
09:44:00 INFO - TEST-PASS | leakcheck | tab process: no leaks detected!
09:44:00 INFO -
09:44:00 INFO - == BloatView: ALL (cumulative) LEAK AND BLOAT STATISTICS, tab process 5836
09:44:00 INFO -
09:44:00 INFO - |<----------------Class--------------->|<-----Bytes------>|<----Objects---->|
09:44:00 INFO - | | Per-Inst Leaked| Total Rem|
09:44:00 INFO - 0 |TOTAL | 34 0| 65496 0|
09:44:00 INFO -
09:44:00 INFO - nsTraceRefcnt::DumpStatistics: 691 entries
09:44:00 INFO -
09:44:00 INFO - TEST-PASS | leakcheck | tab process: no leaks detected!
09:44:00 INFO -
09:44:00 INFO - == BloatView: ALL (cumulative) LEAK AND BLOAT STATISTICS, tab process 5840
09:44:00 INFO -
09:44:00 INFO - |<----------------Class--------------->|<-----Bytes------>|<----Objects---->|
09:44:00 INFO - | | Per-Inst Leaked| Total Rem|
09:44:00 INFO - 0 |TOTAL | 30 0| 91138 0|
09:44:00 INFO -
09:44:00 INFO - nsTraceRefcnt::DumpStatistics: 705 entries
09:44:00 INFO -
09:44:00 INFO - TEST-PASS | leakcheck | tab process: no leaks detected!
09:44:00 INFO -
09:44:00 INFO - == BloatView: ALL (cumulative) LEAK AND BLOAT STATISTICS, tab process 5864
09:44:00 INFO -
09:44:00 INFO - |<----------------Class--------------->|<-----Bytes------>|<----Objects---->|
09:44:00 INFO - | | Per-Inst Leaked| Total Rem|
09:44:00 INFO - 0 |TOTAL | 32 0| 83320 0|
09:44:00 INFO -
09:44:00 INFO - nsTraceRefcnt::DumpStatistics: 690 entries
09:44:00 INFO -
09:44:00 INFO - TEST-PASS | leakcheck | tab process: no leaks detected!
09:44:00 INFO -
09:44:00 INFO - == BloatView: ALL (cumulative) LEAK AND BLOAT STATISTICS, tab process 5988
09:44:00 INFO -
09:44:00 INFO - |<----------------Class--------------->|<-----Bytes------>|<----Objects---->|
09:44:00 INFO - | | Per-Inst Leaked| Total Rem|
09:44:00 INFO - 0 |TOTAL | 30 0| 96605 0|
09:44:00 INFO -
09:44:00 INFO - nsTraceRefcnt::DumpStatistics: 775 entries
09:44:00 INFO -
09:44:00 INFO - TEST-PASS | leakcheck | tab process: no leaks detected!
Flags: needinfo?(lgreco)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
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
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
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
Comment 13•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
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)
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).
Flags: needinfo?(cosmin.muntean)
Comment 16•8 years ago
|
||
Please request Beta approval on this when you get a chance.
Assignee | ||
Comment 17•8 years ago
|
||
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?
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+
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
Comment 20•8 years ago
|
||
bugherder uplift |
Flags: in-testsuite+
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.
Flags: qe-verify+
You need to log in
before you can comment on or make changes to this bug.
Description
•