Closed Bug 1350324 Opened 3 years ago Closed 3 years ago

Tabs permanently go into 'busy' state (grey spinner on the tab in the tabstrip) when dragged to another window in some cases

Categories

(Firefox :: Tabbed Browser, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 55
Tracking Status
firefox52 --- unaffected
firefox53 --- wontfix
firefox54 --- verified
firefox55 --- verified

People

(Reporter: wcpan, Assigned: mconley)

References

Details

(Whiteboard: [e10s-multi:+] )

Attachments

(1 file)

Only happens when dom.ipc.processCount > 1, on both Windows and Mac, and from m-c to m-r.

1. open a tab
2. drags it to another Firefox window
3. the tab icon becomes busy
I can't reproduce (windows, local m-c build from sometime last week). Have you tried this on a clean profile? Does it work with e.g. about:newtab, or mozilla.org, or about:support (parent process pages) specifically?

Also, I'm assuming from your description that you mean the icon in the tab element in the tabstrip (not the in-content spinner), and that it doesn't at any point revert back to the favicon (ie it spins indefinitely). Or do I misunderstand?
Flags: needinfo?(wpan)
(In reply to :Gijs from comment #1)
> I can't reproduce (windows, local m-c build from sometime last week). Have
> you tried this on a clean profile? Does it work with e.g. about:newtab, or
> mozilla.org, or about:support (parent process pages) specifically?

I think I've found a way to ensure it can reproduce:

1. open with a clean profile
2. open 2 www.google.com tabs
3. drag one of the tab out and let it opens a new window

> Also, I'm assuming from your description that you mean the icon in the tab
> element in the tabstrip (not the in-content spinner), and that it doesn't at
> any point revert back to the favicon (ie it spins indefinitely). Or do I
> misunderstand?

Yes, I mean the favicon is always spinning after create or move to a window.
Flags: needinfo?(wpan)
I could reproduce based on comment #2 after a few tries (tested on OS X this time). Not sure what governs this, I guess there's a race of sorts. Mike, this feels like your area of interest? Let me know if you know other people to recommend look at this, I don't want to pile too much stuff on your plate. :-(
Flags: needinfo?(mconley)
Summary: Tab becomes busy when it drags to another window → Tabs permanently go into 'busy' state when dragged to another window in some cases
Summary: Tabs permanently go into 'busy' state when dragged to another window in some cases → Tabs permanently go into 'busy' state (grey spinner on the tab in the tabstrip) when dragged to another window in some cases
Will hopefully get a chance to look at this soon.
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:55.0) Gecko/20100101 Firefox/55.0

I have tested this issue on Windows 10 x64, using the STR provided in the comment 2, performed a regression, the result as follows:

Last good revision: 13b48d5e00f4b98718f2a16cac1b2ae2bc7c00c1
First bad revision: aefa445b9c775c92a31f7f21f4abf07b6a7e2caf
Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=13b48d5e00f4b98718f2a16cac1b2ae2bc7c00c1&tochange=aefa445b9c775c92a31f7f21f4abf07b6a7e2caf

Looks like the following bug has the changes which introduced the regression:
https://bugzilla.mozilla.org/show_bug.cgi?id=1303113

Blake can you please take a look at this?

Note: I have also tested this on the latest Nightly (55.0a1-20170402030202), with e10s disabled and the issue was not reproducible.
Whiteboard: [e10s-multi:+]
Duplicate of this bug: 1353945
Blocks: 1336398
See Also: → 1353738
I think Firefox 52 is also affected if dom.ipc.processCount > 1 (of course with e10s enabled).
Maybe it's only me.
Going to spend some time looking at this today.
Still piecing together what's going on here. Hopefully some answers today...
I have a solution baking on try. See commit message for details.
Flags: needinfo?(mrbkap)
Flags: needinfo?(mconley)
Attachment #8856719 - Flags: review?(gijskruitbosch+bugs)
I wonder if this is related to about:blank-related intermittents I've been fighting for the past month or so. Did we start sending state change events even after a nsIDocShell::Stop() call or something?
Comment on attachment 8856719 [details]
Bug 1350324 - Be more rigorous about ignoring the initial about:blank load for tab progress listeners.

https://reviewboard.mozilla.org/r/128646/#review131908

Nice work!

::: browser/base/content/tabbrowser.xml:587
(Diff revision 1)
> +              if (aStateFlags & nsIWebProgressListener.STATE_STOP &&
> +                  this.mRequestCount == 0) {

Should we sanity-check aLocation here as well, to be safe?
Attachment #8856719 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to Blake Kaplan (:mrbkap) from comment #13)
> I wonder if this is related to about:blank-related intermittents I've been
> fighting for the past month or so. Did we start sending state change events
> even after a nsIDocShell::Stop() call or something?

Seems to be the case. Here's a stack if you're curious:

XUL`nsBrowserStatusFilter::OnStateChange(this=0x000000012e35f900, aWebProgress=0x000000012c077000, aRequest=0x000000013d1fcd80, aStateFlags=65552, aStatus=<unavailable>) + 379 at nsBrowserStatusFilter.cpp:172
    frame #17: 0x000000010fdcbb6c XUL`nsDocLoader::DoFireOnStateChange(this=0x000000012c077000, aProgress=0x000000012c077000, aRequest=0x000000013d1fcd80, aStateFlags=0x00007fff50e4324c, aStatus=2152398850) + 300 at nsDocLoader.cpp:1276
    frame #18: 0x000000010fdcb412 XUL`nsDocLoader::FireOnStateChange(this=<unavailable>, aProgress=0x000000012c077000, aRequest=0x000000013d1fcd80, aStateFlags=65552, aStatus=2152398850) + 226 at nsDocLoader.cpp:1239
    frame #19: 0x000000010fdcb17f XUL`nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, nsresult) [inlined] nsDocLoader::doStopURLLoad(this=0x000000012c077000, request=0x000000013d1fcd80, aStatus=2152398850) + 30 at nsDocLoader.cpp:817
    frame #20: 0x000000010fdcb161 XUL`nsDocLoader::OnStopRequest(this=0x000000012c077000, aRequest=0x000000013d1fcd80, aCtxt=<unavailable>, aStatus=2152398850) + 833 at nsDocLoader.cpp:613
    frame #21: 0x000000010fdcb57d XUL`non-virtual thunk to nsDocLoader::OnStopRequest(this=<unavailable>, aRequest=<unavailable>, aCtxt=<unavailable>, aStatus=<unavailable>) + 13 at nsDocLoader.cpp:487
    frame #22: 0x000000010f54c0bf XUL`mozilla::net::nsLoadGroup::RemoveRequest(this=0x0000000121463790, request=0x000000013d1fcd80, ctxt=0x0000000000000000, aStatus=2152398850) + 1023 at nsLoadGroup.cpp:629
    frame #23: 0x000000010f54aee4 XUL`mozilla::net::nsLoadGroup::Cancel(this=0x0000000121463790, status=2152398850) + 580 at nsLoadGroup.cpp:265
    frame #24: 0x000000010fdca457 XUL`nsDocLoader::Stop(this=0x000000012c077000) + 295 at nsDocLoader.cpp:245
    frame #25: 0x00000001122797f8 XUL`nsDocShell::Stop(unsigned int) [inlined] nsDocShell::Stop(this=<unavailable>) + 328 at nsDocShell.h:190
    frame #26: 0x00000001122797f0 XUL`nsDocShell::Stop(this=0x000000012c077000, aStopFlags=1) + 320 at nsDocShell.cpp:5564
    frame #27: 0x0000000112285622 XUL`nsDocShell::InternalLoad(this=0x000000012c077000, aURI=0x000000012df9f980, aOriginalURI=0x0000000000000000, aLoadReplace=false, aReferrer=0x0000000000000000, aReferrerPolicy=0, aTriggeringPrincipal=0x00000000000001f2, aPrincipalToInherit=<unavailable>, aFlags=<unavailable>, aWindowTarget=<unavailable>, aTypeHint=<unavailable>, aFileName=<unavailable>, aPostData=<unavailable>, aHeadersData=<unavailable>, aLoadType=<unavailable>, aSHEntry=0x0001000150e437a0, aFirstParty=<unavailable>, aSrcdoc=<unavailable>, aSourceDocShell=0x000000010edc7df7, aBaseURI=<unavailable>, aCheckForPrerender=true, aDocShell=0x000000013e0a6000, aRequest=0x0000000000003000) + 10418 at nsDocShell.cpp:10698
    frame #28: 0x00000001122824bd XUL`nsDocShell::LoadURI(this=0x000000012c077000, aURI=0x000000012df9f980, aLoadInfo=<unavailable>, aLoadFlags=<unavailable>, aFirstParty=false) + 2877 at nsDocShell.cpp:1570
    frame #29: 0x00000001102cb376 XUL`nsFrameLoader::ReallyStartLoadingInternal(this=0x000000012eb38c10) + 1222 at nsFrameLoader.cpp:874
    frame #30: 0x00000001102a0d38 XUL`nsDocument::MaybeInitializeFinalizeFrameLoaders() [inlined] nsFrameLoader::ReallyStartLoading(this=0x000000012eb38c10) + 8 at nsFrameLoader.cpp:758
    frame #31: 0x00000001102a0d30 XUL`nsDocument::MaybeInitializeFinalizeFrameLoaders(this=0x000000012f11e000) + 448 at nsDocument.cpp:7254
    frame #32: 0x0000000111426c2a XUL`mozilla::dom::XULDocument::DoneWalking(this=0x000000012f11e000) + 890 at XULDocument.cpp:3036
    frame #33: 0x000000011142709f XUL`non-virtual thunk to mozilla::dom::XULDocument::StyleSheetLoaded(mozilla::StyleSheet*, bool, nsresult) [inlined] mozilla::dom::XULDocument::StyleSheetLoaded(mozilla::StyleSheet*, bool, nsresult) + 47 at XULDocument.cpp:3128
    frame #34: 0x000000011142707f XUL`non-virtual thunk to mozilla::dom::XULDocument::StyleSheetLoaded(this=<unavailable>, aSheet=<unavailable>, aWasAlternate=<unavailable>, aStatus=<unavailable>) + 15 at XULDocument.cpp:3115
    frame #35: 0x000000011165079e XUL`mozilla::css::Loader::SheetComplete(this=0x000000012b24ad40, aLoadData=<unavailable>, aStatus=NS_OK) + 446 at Loader.cpp:1842
    frame #36: 0x000000011164caff XUL`mozilla::css::SheetLoadData::Run() [inlined] mozilla::css::Loader::HandleLoadEvent(this=0x000000012b24ad40, aEvent=<unavailable>) + 97 at Loader.cpp:2538
    frame #37: 0x000000011164ca9e XUL`mozilla::css::SheetLoadData::Run(this=<unavailable>) + 14 at Loader.cpp:418
    frame #38: 0x000000010f4be112 XUL`nsThread::ProcessNextEvent(this=0x000000010f0fa1c0, aMayWait=<unavailable>, aResult=0x00007fff50e443a7) + 1234 at nsThread.cpp:1269
    frame #39: 0x000000010f4bc42e XUL`NS_ProcessPendingEvents(aThread=<unavailable>, aTimeout=10) + 78 at nsThreadUtils.cpp:331
    frame #40: 0x0000000111508cb1 XUL`nsBaseAppShell::NativeEventCallback(this=0x000000011eb3d7d0) + 113 at nsBaseAppShell.cpp:97
    frame #41: 0x00000001115639f9 XUL`nsAppShell::ProcessGeckoEvents(aInfo=0x000000011eb3d7d0) + 297 at nsAppShell.mm:399
    frame #42: 0x00007fff85b6ba01 CoreFoundation`__CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17
    frame #43: 0x00007fff85b5db8d CoreFoundation`__CFRunLoopDoSources0 + 269
    frame #44: 0x00007fff85b5d1bf CoreFoundation`__CFRunLoopRun + 927
    frame #45: 0x00007fff85b5cbd8 CoreFoundation`CFRunLoopRunSpecific + 296
    frame #46: 0x00007fff9260256f HIToolbox`RunCurrentEventLoopInMode + 235
    frame #47: 0x00007fff926021ee HIToolbox`ReceiveNextEventCommon + 179
    frame #48: 0x00007fff9260212b HIToolbox`_BlockUntilNextEventMatchingListInModeWithFilter + 71
    frame #49: 0x00007fff8d2768ab AppKit`_DPSNextEvent + 978
    frame #50: 0x00007fff8d275e58 AppKit`-[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] + 346
    frame #51: 0x0000000111563106 XUL`-[GeckoNSApplication nextEventMatchingMask:untilDate:inMode:dequeue:](self=0x000000010f038a10, _cmd=<unavailable>, mask=18446744073709551615, expiration=4001-01-01 00:00:00 UTC, mode=@"kCFRunLoopDefaultMode", flag=YES) + 86 at nsAppShell.mm:130
    frame #52: 0x00007fff8d26baf3 AppKit`-[NSApplication run] + 594
    frame #53: 0x0000000111563f5a XUL`nsAppShell::Run(this=<unavailable>) + 234 at nsAppShell.mm:673
    frame #54: 0x0000000112543809 XUL`nsAppStartup::Run(this=0x000000011edca470) + 41 at nsAppStartup.cpp:283
    frame #55: 0x00000001125cf2f4 XUL`XREMain::XRE_mainRun(this=<unavailable>) + 3972 at nsAppRunner.cpp:4538
    frame #56: 0x00000001125cfaa0 XUL`XREMain::XRE_main(this=0x00007fff50e45ef0, argc=<unavailable>, argv=<unavailable>, aConfig=<unavailable>) + 1456 at nsAppRunner.cpp:4718
    frame #57: 0x00000001125cfec4 XUL`XRE_main(argc=<unavailable>, argv=<unavailable>, aConfig=<unavailable>) + 228 at nsAppRunner.cpp:4811
    frame #58: 0x000000010edba0d5 firefox`main [inlined] do_main(argc=<unavailable>, argv=<unavailable>, envp=0x00007fff50e46540) + 349 at nsBrowserApp.cpp:236
    frame #59: 0x000000010edb9f78 firefox`main(argc=<unavailable>, argv=<unavailable>, envp=0x00007fff50e46540) + 552 at nsBrowserApp.cpp:307
    frame #60: 0x000000010edb9d44 firefox`start + 52
Comment on attachment 8856719 [details]
Bug 1350324 - Be more rigorous about ignoring the initial about:blank load for tab progress listeners.

https://reviewboard.mozilla.org/r/128646/#review131908

> Should we sanity-check aLocation here as well, to be safe?

Sounds good, thanks!
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6cc7765e90bf
Be more rigorous about ignoring the initial about:blank load for tab progress listeners. r=Gijs
https://hg.mozilla.org/mozilla-central/rev/6cc7765e90bf
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Is this a thing we'd need in 54?
Flags: needinfo?(mrbkap)
Yes.
Flags: needinfo?(mrbkap)
Comment on attachment 8856719 [details]
Bug 1350324 - Be more rigorous about ignoring the initial about:blank load for tab progress listeners.

Approval Request Comment
[Feature/Bug causing the regression]:

e10s-multi

[User impact if declined]:

Users who drag out or in tabs that are running out of process might find that the tab shows the "connecting" throbber despite the page load being finished. This doesn't affect the user's ability to browse, nor break the tab. The next load in the tab should reset the state.

[Is this code covered by automated tests?]:

Tab drag in and drag out is, but this particular bug does not have a regression test.

[Has the fix been verified in Nightly?]:

Not yet, but I'll needinfo? myself to do that.

[Needs manual test from QE? If yes, steps to reproduce]: 

Would be a good idea, yeah. STR are in comment 2.

[List of other uplifts needed for the feature/fix]:

None.

[Is the change risky?]:

No.

[Why is the change risky/not risky?]:

We're making the part of the browser front-end that pays attention to state changes in the tab more intelligent by skipping updates that occur very very early on in a tab's lifetime.

[String changes made/needed]:

None.
Flags: needinfo?(mconley)
Attachment #8856719 - Flags: approval-mozilla-beta?
mconley rules!
Assignee: nobody → mconley
Hi :emilpasca, 
Can you help verify if this issue was fixed as expected on a latest Nightly build? Thanks!
Flags: qe-verify+
Flags: needinfo?(emil.pasca)
Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:55.0) Gecko/20100101 Firefox/55.0

Verified as fixed on the latest Nightly(55.0a1-20170418030220) on Windows 10 x64, Windows 7 x64, Mac OS X 10.12 and Ubuntu 14.04 x64.
Flags: needinfo?(emil.pasca)
And now it's been verified (comment 25) - thanks emilpasca!
Flags: needinfo?(mconley)
Comment on attachment 8856719 [details]
Bug 1350324 - Be more rigorous about ignoring the initial about:blank load for tab progress listeners.

Fix a tab dragging issue and was verified. Beta54+.
Attachment #8856719 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I reproduced this issue using Fx 55.0a1 (2017-03-21) on Windows 10 x64 with STR from description and comment 2.
I can confirm this issue is fixed, I verified using Fx 54.0b2 on Windows 10 x64, Windows 7 x32 and Mac OS X 10.11.6. 

Cheers!
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.