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

Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)

Reporter

Description

•

7 years ago

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

:Gijs (he/him)

Comment 1

•

7 years ago

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)

Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)

Reporter

Comment 2

•

7 years ago

(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)

:Gijs (he/him)

Comment 3

•

7 years ago

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

:Gijs (he/him)

Updated

•

7 years ago

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

Mike Conley (:mconley) (:⚙️)

Assignee

Comment 5

•

7 years ago

Will hopefully get a chance to look at this soon.

Keywords: regressionwindow-wanted

Emil Pasca [:emilpasca]

Comment 6

•

7 years ago

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.

status-firefox52: --- → unaffected

status-firefox53: --- → affected

status-firefox54: --- → affected

status-firefox55: --- → affected

Flags: needinfo?(mrbkap)

Keywords: regressionwindow-wanted

Erin Lancaster [:elan]

Updated

•

7 years ago

Whiteboard: [e10s-multi:+]

:Gijs (he/him)

Updated

•

7 years ago

Blocks: 1336398

Comment 8

•

7 years ago

I think Firefox 52 is also affected if dom.ipc.processCount > 1 (of course with e10s enabled).
Maybe it's only me.

Mike Conley (:mconley) (:⚙️)

Assignee

Comment 9

•

7 years ago

Going to spend some time looking at this today.

Mike Conley (:mconley) (:⚙️)

Assignee

Comment 10

•

7 years ago

Still piecing together what's going on here. Hopefully some answers today...

Comment hidden (mozreview-request)

The initial about:blank that we load in a new <xul:browser> is not a load we usually
care about. We have code in tabbrowser.xml that ignores the state changes for that
initial load, but I guess it's not perfect - there's a case where we were getting
a STATE_STOP for that initial about:blank load (since the DocShell will call
Stop() on itself before loading that initial about:blank), and then a STATE_START
once the about:blank actually started to load. The initial STATE_STOP was ignored,
but cleared the "ignore" flag for the STATE_START to come through and show the
loading throbber. Then, when flipping remoteness, the "busy" state was not cleared.

This patch makes it so that we ignore that initial STATE_STOP (differentiated from
other STATE_STOPS because we notice that no loads were currently in progress).

Review commit: https://reviewboard.mozilla.org/r/128646/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/128646/

Mike Conley (:mconley) (:⚙️)

Assignee

Comment 12

•

7 years ago

I have a solution baking on try. See commit message for details.

Flags: needinfo?(mrbkap)

Flags: needinfo?(mconley)

Mike Conley (:mconley) (:⚙️)

Assignee

Updated

•

7 years ago

Attachment #8856719 - Flags: review?(gijskruitbosch+bugs)

Blake Kaplan (:mrbkap) (inactive)

Comment 13

•

7 years ago

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?

:Gijs (he/him)

Comment 14

•

7 years ago

mozreview-review

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+

Mike Conley (:mconley) (:⚙️)

Assignee

Comment 15

•

7 years ago

(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

Mike Conley (:mconley) (:⚙️)

Assignee

Comment 16

•

7 years ago

mozreview-review-reply

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!

Comment hidden (mozreview-request)

Comment on attachment 8856719 [details]
Bug 1350324 - Be more rigorous about ignoring the initial about:blank load for tab progress listeners.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/128646/diff/1-2/

Pulsebot

Comment 18

•

7 years ago

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

Carsten Book [:Tomcat]

Comment 19

•

7 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/6cc7765e90bf

Status: NEW → RESOLVED

Closed: 7 years ago

status-firefox55: affected → fixed

Resolution: --- → FIXED

Target Milestone: --- → Firefox 55

Mike Conley (:mconley) (:⚙️)

Assignee

Comment 20

•

7 years ago

Is this a thing we'd need in 54?

Flags: needinfo?(mrbkap)

Blake Kaplan (:mrbkap) (inactive)

Comment 21

•

7 years ago

Yes.

Flags: needinfo?(mrbkap)

Mike Conley (:mconley) (:⚙️)

Assignee

Comment 22

•

7 years ago

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?

Blake Kaplan (:mrbkap) (inactive)

Comment 23

•

7 years ago

mconley rules!

Assignee: nobody → mconley

Gerry Chang [:gchang]

Comment 24

•

7 years ago

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)

Emil Pasca [:emilpasca]

Comment 25

•

7 years ago

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.

status-firefox55: fixed → verified

Flags: needinfo?(emil.pasca)

Mike Conley (:mconley) (:⚙️)

Assignee

Comment 26

•

7 years ago

And now it's been verified (comment 25) - thanks emilpasca!

Flags: needinfo?(mconley)

Gerry Chang [:gchang]

Comment 27

•

7 years ago

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+

Carsten Book [:Tomcat]

Comment 28

•

7 years ago

bugherder uplift

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

status-firefox54: affected → fixed

Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact mboldan@mozilla.com

Comment 29

•

7 years ago

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

status-firefox54: fixed → verified

Flags: qe-verify+

Liz Henry (:lizzard) (relman/hg->git project)

Updated

•

7 years ago

status-firefox53: affected → wontfix