1402493 - Refresh button gets stuck in disabled stop state when extension loads content in page

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect, P1)

Description

[u602702]

Attachment: 2017-09-22_17-17-00.gif

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20170917031738

Steps to reproduce:

Install "Reddit Enhancment suite and open any image, it will make the refresh button unresponsive and unable to be clicked. If you don't load anything, the button works as intended, as does it without the plugin. 

This is a very popular plugin and this could be affecting quite a few people.


Actual results:

The refresh button stops working when an image is loaded, forcing use of f5 or ctrl-r.


Expected results:

The refresh button should be able to be clicked after the image has finished loading.

Comment 1

[Kestrel]

Regression range between 2017-08-01 and 2017-08-04, most likely regressed by Bug 1386255.

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

[Tracking Requested - why for this release]: add-on related issue that can cause the browser chrome to get in a broken state

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

I have confirmed via mozregression that bug 1386255 is the cause of this.

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Bug 1402493 - Don't prevent switching back to Reload for top level about: loads.

In the steps to reproduce of this bug, we get see a load start for about:blank that is not top level. We then see a following load complete (STATE_STOP) that has aWebProgress.isTopLevel=true. This code is really here to prevent flickering from reload->stop->reload for about:home and other about: pages. I don't see any harm in being more agressive when switching back to Reload since that is the default state.

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

Comment 5

[:Gijs (he/him)]

Comment on attachment 8912919 [details]
Bug 1402493 - Don't prevent switching back to Reload for top level about: loads.

https://reviewboard.mozilla.org/r/184230/#review189696

Tentative r+, see comment below.

::: browser/base/content/browser.js:5057
(Diff revision 1)
>      }
>      this.reload.setAttribute("displaystop", "true");
>    },
>  
>    switchToReload(aRequest, aWebProgress) {
> -    if (!this.ensureInitialized() || !this._shouldSwitch(aRequest, aWebProgress) ||
> +    if (!this.ensureInitialized() || !this.reload.hasAttribute("displaystop")) {

Your reasoning makes sense, but are you sure we should do this even if there's no request or the request has no URI at all? It doesn't look like we check that right now, and neither does the caller.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8912919 [details]
Bug 1402493 - Don't prevent switching back to Reload for top level about: loads.

https://reviewboard.mozilla.org/r/184230/#review189696

> Your reasoning makes sense, but are you sure we should do this even if there's no request or the request has no URI at all? It doesn't look like we check that right now, and neither does the caller.

The _shouldSwitch function returns true if there is no aRequest and it also returns true if there is no aRequest.originalURI, so this isn't a change in behavior for those two inputs.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e595d08c3ee2
Don't prevent switching back to Reload for top level about: loads. r=Gijs

Comment 8

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #6)
> Comment on attachment 8912919 [details]
> Bug 1402493 - Don't prevent switching back to Reload for top level about:
> loads.
> 
> https://reviewboard.mozilla.org/r/184230/#review189696
> 
> > Your reasoning makes sense, but are you sure we should do this even if there's no request or the request has no URI at all? It doesn't look like we check that right now, and neither does the caller.
> 
> The _shouldSwitch function returns true if there is no aRequest and it also
> returns true if there is no aRequest.originalURI, so this isn't a change in
> behavior for those two inputs.

Ah, of course it does - clearly I misread it. Down with 6-line conditionals, I say! ;-)

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 10

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Jared, should we uplift this to Beta57?

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8912919 [details]
Bug 1402493 - Don't prevent switching back to Reload for top level about: loads.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1386255
[User impact if declined]: stop/reload button may get stuck on stop
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: reverts to behavior that we had for the stop->reload switching pre-57.
[String changes made/needed]: none

Comment 12

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8912919 [details]
Bug 1402493 - Don't prevent switching back to Reload for top level about: loads.

Photon related, Beta57+

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0fe06c5dd395

Comment 14

[marius.santa]

Attachment: refresh available.gif

This bug is verified on Firefox 57.0b5 (20171002181526) and Firefox 58.0a1 (20171002220204) under Windows 10 64bit/Mac OS X 10.12.3.

Please see the attached video.