1386255 - photon animated refresh button gets confused and gets stuck on disabled stop button

Status: VERIFIED FIXED

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

Description

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Screenshot 2017-08-01 09.47.54.png

STR:

1. Open a new nightly
2. Load this page in a new tab: https://beta.doodle.com/poll/w8zhuavpk6ya4kwr
3. If you get a first time overlay, dismiss and go back to (2)

About 50% of the time I end up on a disabled stop button on this page.  I noticed because I wanted to reload, but the button wasn't available.

See attached screenshot.

I'm on windows 10 using 56.0a1 (2017-08-01) (64-bit).

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Justin, who is the right person to ping for refresh/stop button animation changes?  I know you have blogged a bit about it in your photon newsletter.  Thanks.

Comment 2

[:Gijs (he/him)]

(In reply to Ben Kelly [:bkelly] from comment #1)
> Justin, who is the right person to ping for refresh/stop button animation
> changes?  I know you have blogged a bit about it in your photon newsletter. 
> Thanks.

Jared. :-)

Comment 3

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

Using mozregression I tracked it down to this pushlog,
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=0faada5c2f308f101ab7c54a87b3dce80b97d0e3&tochange=417eea89f792059e88be5e217e6f1a5cecf2d3d0

Of those, https://bugzilla.mozilla.org/show_bug.cgi?id=1376892 looks like the cause.

Perry, can you look in to this?

Comment 4

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

[Tracking Requested - why for this release]: Some webpages may cause the stop/reload button to stop working.

Comment 5

[Perry Jiang [:perry] [no longer employee, use ni?]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #3)
> Using mozregression I tracked it down to this pushlog,
> https://hg.mozilla.org/mozilla-central/
> pushloghtml?fromchange=0faada5c2f308f101ab7c54a87b3dce80b97d0e3&tochange=417e
> ea89f792059e88be5e217e6f1a5cecf2d3d0
> 
> Of those, https://bugzilla.mozilla.org/show_bug.cgi?id=1376892 looks like
> the cause.
> 
> Perry, can you look in to this?

Will do.

Comment 6

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

Actually, I think it might be bug 1375599 which caused this.

Comment 7

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

I have confirmed via local backout that bug 1375599 did not cause this. Also, adding `return true;` at the top of CombinedStopReload._shouldSwitch fixes the bug for me.

Comment 8

[Perry Jiang [:perry] [no longer employee, use ni?]]

I'm not able to reproduce this on Windows 10 64-bit with Nightly 56.0a1 (2017-08-01). To clarify, this can be seen by opening a fresh Nightly and going to https://beta.doodle.com/poll/w8zhuavpk6ya4kwr a few times when there's no Doodle loading screen?

Comment 9

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

(In reply to :Perry Jiang from comment #8)
> I'm not able to reproduce this on Windows 10 64-bit with Nightly 56.0a1
> (2017-08-01). To clarify, this can be seen by opening a fresh Nightly and
> going to https://beta.doodle.com/poll/w8zhuavpk6ya4kwr a few times when
> there's no Doodle loading screen?

Yes, I can reproduce it on a clean profile with a local build. Make sure that you are using the Beta and don't get moved back to the non-beta version.

Comment 10

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

At the beginning of _shouldSwitch I put the following log statement:
> console.log(`_shouldSwitch with ${aRequest && aRequest.originalURI && aRequest.originalURI.spec}`);

This gave me the following output:
// Open a new tab and load the page
> _shouldSwitch with https://beta.doodle.com/poll/w8zhuavpk6ya4kwr
> _shouldSwitch with https://beta.doodle.com/poll/w8zhuavpk6ya4kwr
> _shouldSwitch with https://acdn.adnxs.com/ib/static/usersync/v3/async_usersync.html
> _shouldSwitch with https://acdn.adnxs.com/ib/static/usersync/v3/async_usersync.html
> _shouldSwitch with about:srcdoc
> _shouldSwitch with about:srcdoc
> _shouldSwitch with wyciwyg://0/https://beta.doodle.com/poll/w8zhuavpk6ya4kwr
> _shouldSwitch with wyciwyg://1/https://beta.doodle.com/poll/w8zhuavpk6ya4kwr
> _shouldSwitch with about:blank
> _shouldSwitch with about:blank
> _shouldSwitch with wyciwyg://6/https://x.vindicosuite.com/serve/?v=5;m=3;l=529015;c=1004737;b=4448846;ts=1501635174;u=%3Cpage_url_escaped%3E;r=%3Creferrer_escaped%3E;ad=CPekIBCqlA0Y5gsgASgEMNWIATjjvwFAta8TSLe2F1DBqT1YzsSPAmC_twlomvckcAF4A4gBAJABAJgBAKIBEzM5ODU4NDUzMDc2OTE2NTUyMjiyAQhDUkVBVElWRbgBAcABAMgBANABANgBAOgBxcC5g9or;xid=3985845307691655228;ep=1;pasmc=https%3A%2F%2Fnym1-ib.adnxs.com%2Fclick%3FpHA9Ctej0D8jZ2FPO_zNPwAAAIDCdSNAI2dhTzv8zT-kcD0K16PQP97fQ-Eqsdk92eX5oaA4hBtmIoFZAAAAAF1nYwBBDgAAvAQAAGUAAAATWKQDyxYOAAAAAABVU0QAVVNEAKAAWAKmUgAAAAABAQUCAQAAAK4A_iQL-AAAAAA.%2Fbn%3D7048%2Freferrer%3Dhttps%253A%252F%252Fbeta.doodle.com%252Fpoll%252Fw8zhuavpk6ya4kwr%2Fclickenc%3Dhttps%253A%252F%252Fgoogleads.g.doubleclick.net%252Fdbm%252Fclk%253Fsa%253DL%2526ai%253DCgwFtZCKBWYGDJsaxMavapPAE5-qqhUuF6Nbr6ASuusTcpwQQASDml9YlYMmWx4mQpOgPyAEJqAMBqgSrAU_QOPPH0gUI70OKiYUWkbE-rPfmIbi6vLQ9yR65oKTgLHg_mYlDvtn0PU57CezuXJtX3MW2uwrI2CbA1yCX5Gb25M04Lgqhyh1gn6FLG2cwmAU3BrM268oRb-7lHCKxx1_o_R1XWb9JlpNpY4JWB3yo3CzL01QTAdZp9JKJlBgNF5l2uBB3FPhs1lY9BuXJt8Fw2BYsaDs3NBwrLxTitpNOeGWDpDVW_vy91OAEA5AGAaAGTYAH3cSVZKgHr8obqAfdyhuoB6a-G9gHANIIBwiAQBABGADyCA5iaWRkZXItMTg1NzY4NsgT1pL8AdATANgTAw%2526num%253D1%2526pr%253Dappnexuswinpricemacro%2526cid%253DCAASEuRoxX7oczOXTMimTISkV3_ZLg%2526sig%253DAOD64_1ki0WbPBz8nTk3SxD8uxs8d-nzHA%2526client%253Dca-pub-3076890012741467%2526dbm_c%253DAKAmf-DE-80GQEQN_sqSuKwhek-6n0Lop_er5r1ZdUWuc7sWHaOwnLSskiWL_F7OtXSBv9ynuOK2%2526dbm_d%253DAKAmf-C3GwcF5owmWBJvW9Aqea5rgIjd2HKG2H01wXPFrfUpgag11TFoNQ35g7eE0IAhaQkSdwGKWSVh4-pEz14v5l_NE0jI55mu2GN1Na4LumkoQicNbtgysT3H6GAKdBJn0ZOjn8lhz6nPGy3yohI5g3I0I6hQxCkpKyaa2KmU3b93jZAmm6n7IBajN73DZ0_-qdmftZ0N%2526adurl%253D
> _shouldSwitch with about:blank
> _shouldSwitch with wyciwyg://7/https://x.vindicosuite.com/serve/?v=5;m=3;l=529013;c=1004737;b=4448848;ts=1501635174;u=%3Cpage_url_escaped%3E;r=%3Creferrer_escaped%3E;ad=CPWkIBCqlA0Y5gsgASgEMNWIATjjvwFAta8TSLe2F1DBqT1Y0MSPAmDBtwlojfckcAF4A4gBAJABAJgBAKIBEzYwMDg2NDc0MzQ2OTA2MDA4NjWyAQhDUkVBVElWRbgBAcABAMgBANABANgBAOgB48C5g9or;xid=6008647434690600865;ep=1;pasmc=https%3A%2F%2Fnym1-ib.adnxs.com%2Fclick%3FpHA9Ctej0D8jZ2FPO_zNPwAAAIDC9RtAI2dhTzv8zT-kcD0K16PQPyEKo4TzroN72eX5oaA4hBtmIoFZAAAAAIZAYgBBDgAAvAQAAGUAAAAVWKQDyxYOAAAAAABVU0QAVVNEANgCWgD2SQAAAAABAQUCAQAAAK4AGSUe6QAAAAA.%2Fbn%3D7048%2Freferrer%3Dhttps%253A%252F%252Fbeta.doodle.com%252Fpoll%252Fw8zhuavpk6ya4kwr%2Fclickenc%3Dhttps%253A%252F%252Fgoogleads.g.doubleclick.net%252Fdbm%252Fclk%253Fsa%253DL%2526ai%253DCT3yPZCKBWbX5JYmL3QGw_IDID-fqqoVLrejW6-gErrrE3KcEEAEg5pfWJWDJlseJkKToD8gBCagDAaoEqwFP0PUybuceYHAJWZ7fnBFEWSB4FR74PyVrp93a44byP9-PRGM13H4C4kNmxbOFfctGO-yvc--qSuDKHdwe1ETknrhCWmaM42XWckIKFIganASANjRftltDrfps7QGqBLzrnlHpi3JlX9pFpWqfDG31BjnOLPLkTrZbT1jNpkiLprklbZ7LvkSHn9ZnB8bKtW-cTvfMLrMiEEY-huNEAeTO3McGezz30_a4yJfgBAOQBgGgBk2AB93ElWSoB6_KG6gH3cobqAemvhvYBwDSCAcIgEAQARgA8ggOYmlkZGVyLTE4NTc2ODbIE9aS_AHQEwDYEwM%2526num%253D1%2526pr%253Dappnexuswinpricemacro%2526cid%253DCAASEuRohEqUCncku-CkscnmFITkgA%2526sig%253DAOD64_1HKqPYgzfOsvRxsXe2LixcxAekkw%2526client%253Dca-pub-3076890012741467%2526dbm_c%253DAKAmf-DLIpUgPagMr-gkiGFVy0OHq5wLXZG4BPUL60fQx6emlBFYTqAQdvSSS_6KklV3YMbWhrqI%2526dbm_d%253DAKAmf-ALhTJIWXGWTBzd2xC-6qn3xDyS6bZkukqBzWrw5eBom3u6MyBOeKKLmRhucgtX43QZ8bSl0L863CJrJMB6i1niwaXJ8Uy8kelyIcYHTPRb03k9rtvjFSdIKUKaOEz92eePYqd7YXYyuqS0wrr_NeRhzzT5gwzJv1GOGNt-U2Q4Nj18zF0Lb5BSQxIMgcNRu4MUBgbl%2526adurl%253D
> 
> // Close the tab, open a new tab and load the page again
> 
> _shouldSwitch with https://beta.doodle.com/poll/w8zhuavpk6ya4kwr
> _shouldSwitch with https://beta.doodle.com/poll/w8zhuavpk6ya4kwr
> _shouldSwitch with https://acdn.adnxs.com/ib/static/usersync/v3/async_usersync.html
> _shouldSwitch with https://acdn.adnxs.com/ib/static/usersync/v3/async_usersync.html
> _shouldSwitch with about:srcdoc
> _shouldSwitch with about:srcdoc
> _shouldSwitch with wyciwyg://0/https://beta.doodle.com/poll/w8zhuavpk6ya4kwr
> _shouldSwitch with wyciwyg://1/https://beta.doodle.com/poll/w8zhuavpk6ya4kwr
> _shouldSwitch with about:blank
> _shouldSwitch with about:blank

The interesting things I see here are wyciwyg scheme but we don't do anything special with that in _shouldSwitch. Also there is about:srcdoc and about:blank.

Comment 11

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

The other weird thing about this bug is that the reload button looks disabled but after bug 1376893 we shouldn't show it as disabled (greyed out) anymore.

Comment 12

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

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #11)
> The other weird thing about this bug is that the reload button looks
> disabled but after bug 1376893 we shouldn't show it as disabled (greyed out)
> anymore.

Okay, this actually isn't so weird. Bug 1376893 only added new rules for #reload-button, but we're actually seeing a disabled #stop-button, so bug 1376893 doesn't apply here.

Comment 13

[Gerry Chang [:gchang]]

Track 56+ as new regression and might cause breakage for some webpages.

Comment 14

[:Felipe Gomes (needinfo for replies!)]

So could it be solved by only skipping the animation if it's for a top-level about: load?

The webProgress object (that is not passed as a parameter to _shouldSwitch, but could be passed very easily) has an .isTopLevel property that can be used to check this.

Comment 15

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

(In reply to :Felipe Gomes (needinfo me!) from comment #14)
> So could it be solved by only skipping the animation if it's for a top-level
> about: load?
> 
> The webProgress object (that is not passed as a parameter to _shouldSwitch,
> but could be passed very easily) has an .isTopLevel property that can be
> used to check this.

Yeah, that fixes it. Attaching a patch now...

Comment 16

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

Attachment: Bug 1386255 - Switch the stop/reload button if the about: URL loaded is not top-level.

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

Comment 17

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8893125 [details]
Bug 1386255 - Switch the stop/reload button if the about: URL loaded is not top-level.

https://reviewboard.mozilla.org/r/164122/#review169472

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d9420078ea8c
Switch the stop/reload button if the about: URL loaded is not top-level. r=Felipe

Comment 19

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 20

[Stefan [:StefanG_QA]]

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

This bug fix is Verified with latest Nightly 57.0a1.