Closed
Bug 817947
Opened 11 years ago
Closed 11 years ago
Move a background unloaded tab to New Window fails
Categories
(Firefox :: Tabbed Browser, defect)
Tracking
()
VERIFIED
FIXED
Firefox 22
People
(Reporter: tabutils+bugzilla, Assigned: ttaubert)
References
Details
(Keywords: regression)
Attachments
(1 file, 2 obsolete files)
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0 Build ID: 20121119183901 Steps to reproduce: 1. Keep "Don't load tabs until selected" option on 2. Open several tabs 3. Close Firefox and restart 4. Move a background unloaded tab to New Window via Tab Context Menu Actual results: A blank window is opened. The tab remains in the original window. The Error Console reports: Error: NS_ERROR_NOT_IMPLEMENTED: Component returned failure code: 0x80004001 (NS_ERROR_NOT_IMPLEMENTED) [nsIFrameLoaderOwner.swapFrameLoaders] Source file: chrome://global/content/bindings/browser.xml Line: 1155
Last good nightly: 2012-09-05 First bad nightly: 2012-09-06 Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=6705e131aeaa&tochange=0c4fa25f637b
Component: Untriaged → Tabbed Browser
Keywords: regression
Comment 2•11 years ago
|
||
Regression window(m-i) Good (But contents does not load after detached): http://hg.mozilla.org/integration/mozilla-inbound/rev/85d7349e7086 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/18.0 Firefox/18.0 ID:20120904224012 Bad: http://hg.mozilla.org/integration/mozilla-inbound/rev/e5ef9db325d8 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/18.0 Firefox/18.0 ID:20120904225313 Pushlog: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=85d7349e7086&tochange=e5ef9db325d8 Regressed by Bug 776928
Comment 3•11 years ago
|
||
>Good (But contents does not load after detached): I've filed a separate Bug 817959
Comment 5•11 years ago
|
||
Bug 776928 will ship in 20, we should get this fixed before then.
Assignee: nobody → ttaubert
tracking-firefox20:
--- → +
Assignee | ||
Comment 6•11 years ago
|
||
This should also address bug 726275 comment 21.
Attachment #715125 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 7•11 years ago
|
||
Added a test. I wasn't quite sure where to put this test as we need the session restore service to prepare the state and we're then testing a tabbrowser feature.
Attachment #715125 -
Attachment is obsolete: true
Attachment #715125 -
Flags: review?(gavin.sharp)
Attachment #715133 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 8•11 years ago
|
||
A couple of tests are failing, will investigate... https://tbpl.mozilla.org/?tree=Try&rev=893a2541f8ce
Status: NEW → ASSIGNED
Comment 9•11 years ago
|
||
Some random questions I haven't had a chance to look into at all: Do you need to do the getTabState/setTabState before calling _beginRemoveTab? Does the "pending" attributes on tabs get removed immediately as soon as the tab is selected (before the restore is "complete")? Is there any tab state for pending tabs that getTabState/setTabState won't transfer but _swapBrowserDocShells/browser.swapDocShells would? Does setTabState cause the load in the new tab to begin immediately?
Assignee | ||
Comment 10•11 years ago
|
||
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #9) > Do you need to do the getTabState/setTabState before calling _beginRemoveTab? Probably not but the hasAttribute() check needs to be. > Does the "pending" attributes on tabs get removed immediately as soon as the > tab is selected (before the restore is "complete")? Yes, before the actual URL is loaded. > Is there any tab state for pending tabs that getTabState/setTabState won't > transfer but _swapBrowserDocShells/browser.swapDocShells would? No, because those are tabs coming from session restore, waiting to be restored. Those tabs are completely unloaded. > Does setTabState cause the load in the new tab to begin immediately? Yes, if the tab is selected or restore_on_demand=false.
Comment 11•11 years ago
|
||
Does updateCurrentBrowser necessary after you update pending tab with setTabState?
Assignee | ||
Comment 12•11 years ago
|
||
(In reply to onemen.one from comment #11) > Does updateCurrentBrowser necessary after you update pending tab with > setTabState? Yes, that's what swapBrowsersAndCloseOther() when it's done swapping and closing.
Comment 13•11 years ago
|
||
Comment on attachment 715133 [details] [diff] [review] swap tab states (not docShells) when swapBrowsersAndCloseOther() is called with a pending tab (In reply to Tim Taubert [:ttaubert] from comment #10) > > Do you need to do the getTabState/setTabState before calling _beginRemoveTab? > > Probably not but the hasAttribute() check needs to be. OK. Can you put it after the _beginRemoveTab, just to mirror the order of the other code path? So |var isPending = aOtherTab.hasAttribute("pending");| before _beginRemoveTab, if (isPending) { ... } else { ... } after.
Attachment #715133 -
Flags: review?(gavin.sharp) → review+
Comment 14•11 years ago
|
||
(In reply to Tim Taubert [:ttaubert] from comment #8) > A couple of tests are failing, will investigate... > > https://tbpl.mozilla.org/?tree=Try&rev=893a2541f8ce Those are "opened pages" tests. _swapBrowserDocShells calls unregisterOpenPage on the old browser's URL and sets registeredOpenURI on the new one, so I'm guessing you might need to do something like that in this code path too?
Comment 15•11 years ago
|
||
Comment on attachment 715133 [details] [diff] [review] swap tab states (not docShells) when swapBrowsersAndCloseOther() is called with a pending tab (I guess I shouldn't r+ as-is given the test issues)
Attachment #715133 -
Flags: review+ → feedback+
Assignee | ||
Comment 16•11 years ago
|
||
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #14) > Those are "opened pages" tests. _swapBrowserDocShells calls > unregisterOpenPage on the old browser's URL and sets registeredOpenURI on > the new one, so I'm guessing you might need to do something like that in > this code path too? Aha! I'll look into that, thanks for the pointer.
Assignee | ||
Comment 17•11 years ago
|
||
Yup, better: https://tbpl.mozilla.org/?tree=Try&rev=7f1ec16e1948
Attachment #715133 -
Attachment is obsolete: true
Attachment #716566 -
Flags: review?(gavin.sharp)
Comment 18•11 years ago
|
||
I was trying to understand why that change caused that failure. From what I can tell, it failed because previous tests detached tabs into new windows, which left registered open pages around, which later caused browser_tabMatchesInAwesomebar_perwindowpb.js to fail because it compares the registered open pages to the pages it opens. Its error message is a bit confusing, I will file a separate bug to fix that.
Updated•11 years ago
|
Attachment #716566 -
Flags: review?(gavin.sharp) → review+
Assignee | ||
Comment 19•11 years ago
|
||
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #18) > I was trying to understand why that change caused that failure. From what I > can tell, it failed because previous tests detached tabs into new windows, > which left registered open pages around, which later caused > browser_tabMatchesInAwesomebar_perwindowpb.js to fail because it compares > the registered open pages to the pages it opens. Yeah, I think that's exactly what happened. > Its error message is a bit > confusing, I will file a separate bug to fix that. Yes, I agree. Thanks for that.
Assignee | ||
Comment 20•11 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/16ac95df1a8d
Assignee | ||
Comment 21•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/16ac95df1a8d
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22
Comment 22•11 years ago
|
||
Let's get this nominated, and uplifted if low risk.
status-firefox20:
--- → affected
status-firefox21:
--- → affected
status-firefox22:
--- → fixed
tracking-firefox21:
--- → +
Assignee | ||
Comment 23•11 years ago
|
||
This issue is quite an edge-case and shouldn't be too easy to hit. Also I'm not sure that the patch is really low-risk, I'd rather let it ride the trains if we think it's not too serious.
Comment 24•11 years ago
|
||
Thanks Tim, happy to let this ride the trains then if not too easy to hit.
Comment 25•11 years ago
|
||
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20130421 Firefox/22.0 Verified as fixed on Firefox 22 (Build ID:20130421004014).
Comment 26•11 years ago
|
||
Verified as fixed on FF 22 beta 2. Build ID: 20130521223249 User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20100101 Firefox/22.0
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•