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)

x86_64
Windows 7
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 22
Tracking Status
firefox20 - affected
firefox21 - affected
firefox22 --- verified

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
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
Blocks: 776928
Status: UNCONFIRMED → NEW
Ever confirmed: true
>Good (But contents does not load after detached):
I've filed a separate Bug 817959
Bug 776928 will ship in 20, we should get this fixed before then.
Assignee: nobody → ttaubert
Blocks: 817959
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)
A couple of tests are failing, will investigate...

https://tbpl.mozilla.org/?tree=Try&rev=893a2541f8ce
Status: NEW → ASSIGNED
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?
(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.
Does updateCurrentBrowser necessary after you update pending tab with setTabState?
(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 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+
(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 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+
(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.
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.
Attachment #716566 - Flags: review?(gavin.sharp) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/16ac95df1a8d
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22
Let's get this nominated, and uplifted if low risk.
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.
Thanks Tim, happy to let this ride the trains then if not too easy to hit.
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).
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
Depends on: 922454
You need to log in before you can comment on or make changes to this bug.