1591183 - Reload button stays in loading state after a network error

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Mathieu Darse]

Attachment: Screen recording of the issue

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:72.0) Gecko/20100101 Firefox/72.0

Steps to reproduce:

macOS 10.13.6 & Nightly 72.0a1 (2019-10-23) (64 bits)

1. Pull the network plug
2. Open a new web page (I used www.google.fr)

Actual results:

Firefox displays the usual network error page, but the refresh button stays in loading state (it displays the cross to cancel page loading). Switching to another tab then coming back to this one recovers button state.

Expected results:

Firefox should displays the usual network error page, with the refresh button in idle state (the circular arrow to refresh the page).

Comment 1

[YF (Yang)]

STR:
Open any URL to trigger the error page, e.g. http://a1111.mozilla.org/.

Regression range:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=f748a3d2cdf108e9443fd15332efe477c7c398a9&tochange=54b183263967f76fafee777acc04d05be56e5ff0

Comment 2

[Jean-Yves Avenard [:jya]]

More easily reproducable by opening a non-existing URI:

http://www.google.frasdfasdf/

Comment 3

[Jean-Yves Avenard [:jya]]

So prior to DC, we would have fired the following events:
OnStateChange for start document load
OnStateChange for end url load
OnStateChange for end document load
in that order.

With DocumentChannel we now do:
OnStateChange for start document load
OnStateChange start url load
OnStateChange for end url load
OnStateChange for end document load

We have a start event for every end event which seems much more correct.

The extra start url load causes browser.js to stick to CombinedStopReload.SwitchToStop

This is kinda documented in:
https://searchfox.org/mozilla-central/source/uriloader/base/nsIWebProgressListener.idl#32
" * NOTE: For document requests, a second STATE_STOP is generated (see the

• description of STATE_IS_WINDOW for more details)."

But I don't know if it was a design flaw or done on purpose.

Gijs, I'm happy to have a go at it, but I'm not familiar with browser.js so you may want to take over instead.

Thanks

Comment 4

[:Gijs (he/him)]

(In reply to Jean-Yves Avenard [:jya] from comment #3)

With DocumentChannel we now do:
OnStateChange for start document load
OnStateChange start url load
OnStateChange for end url load
OnStateChange for end document load

We have a start event for every end event which seems much more correct.

This isn't what I see in either the browser.js code that is only supposed to get events for the current tab (and update the reload/stop button) or in the tabs progress listener (which gets events for all tabs). Where are you listening / checking for events?

Comment 5

[:Gijs (he/him)]

(In reply to :Gijs (he/him) from comment #4)

This isn't what I see in either the browser.js code that is only supposed to get events for the current tab (and update the reload/stop button) or in the tabs progress listener (which gets events for all tabs). Where are you listening / checking for events?

Specifically: in browser.js's onStateChange, I get no stop notifications at all. in tabbrowser.js, I get the network one.

That said, I think the culprit is: https://searchfox.org/mozilla-central/rev/9df2ccc256fe54c314b34e52920471a0d04b283e/browser/base/content/tabbrowser.js#5642-5647 .

Comment 6

[:Gijs (he/him)]

The new url start request is throwing us off - it increments mRequestCount, which we expect to only happen if docshell starts a keyword search load in response to the unknown host error (you can see this if you flip browser.fixup.dns_first_for_single_words to true and put in a non-existing hostname like "adfgsdfvsdfgsdbfasdfihsdfggdf").

This should be fixable by only incrementing mRequestCount for STATE_START notifications that have STATE_IS_NETWORK.

Comment 7

[:Gijs (he/him)]

(In reply to Jean-Yves Avenard [:jya] from comment #3)

OnStateChange start url load

FWIW, I'm still curious what you mean by this - I don't see a STATE_IS_URL or anything like that in the docs...

Comment 8

[:Gijs (he/him)]

Oops, ni for comment #7.

Comment 9

[:Gijs (he/him)]

Attachment: Bug 1591183 - fix tabbrowser's expectations about state_start with documentchannel and add a test, r?dao

Comment 10

[Jean-Yves Avenard [:jya]]

(In reply to :Gijs (he/him) from comment #4)

(In reply to Jean-Yves Avenard [:jya] from comment #3)

With DocumentChannel we now do:
OnStateChange for start document load
OnStateChange start url load

OnStateChange for end url load
OnStateChange for end document load

We have a start event for every end event which seems much more correct.

This isn't what I see in either the browser.js code that is only supposed to get events for the current tab (and update the reload/stop button) or in the tabs progress listener (which gets events for all tabs). Where are you listening / checking for events?

I see those with
MOZ_LOG=DocLoader:5

Specifically the call to:
https://searchfox.org/mozilla-central/source/uriloader/base/nsDocLoader.cpp#855
And
https://searchfox.org/mozilla-central/source/uriloader/base/nsDocLoader.cpp#872
Same for the document load.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fc352339bd16
fix tabbrowser's expectations about state_start with documentchannel and add a test, r=dao

Comment 12

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/fc352339bd16

Comment 13

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fc352339bd16