Closed Bug 1591183 Opened 5 years ago Closed 5 years ago

Reload button stays in loading state after a network error

Categories

(Firefox :: Tabbed Browser, defect)

72 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 72
Tracking Status
firefox-esr68 --- unaffected
firefox70 --- unaffected
firefox71 --- unaffected
firefox72 --- fixed

People

(Reporter: hello, Assigned: Gijs)

References

(Regression)

Details

(Keywords: nightly-community, regression)

Attachments

(2 files)

Attached video Screen recording of the issue (deleted) —

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

Status: UNCONFIRMED → NEW
Has Regression Range: --- → yes
Has STR: --- → yes
Ever confirmed: true
OS: Unspecified → All
Regressed by: 1583700
Hardware: Unspecified → All
Summary: Refresh button stays in loading state after a network error → Reload button stays in loading state after a network error
Assignee: nobody → jyavenard

More easily reproducable by opening a non-existing URI:

http://www.google.frasdfasdf/

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

Flags: needinfo?(gijskruitbosch+bugs)

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

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(jyavenard)

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

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.

Assignee: jyavenard → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Flags: needinfo?(jyavenard)

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

Flags: needinfo?(gijskruitbosch+bugs)

Oops, ni for comment #7.

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(jyavenard)

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

Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/autoland/rev/fc352339bd16 fix tabbrowser's expectations about state_start with documentchannel and add a test, r=dao
Component: Untriaged → Tabbed Browser
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 72
Flags: needinfo?(jyavenard)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: