New issue
Advanced search Search tips

Issue 638277 link

Starred by 3 users

Issue metadata

Status: Verified
Owner:
Closed: Aug 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 1
Type: Bug-Regression



Sign in to add a comment

Unresponsive stop(x) button: Pages would never stop loading, even after pressing the stop button

Project Member Reported by cma...@chromium.org, Aug 16 2016

Issue description

Version: 53.0.2774.2
iOS Version: 9.3.3
Device: iPad, not sure about iPhone.


1. Surf, create tabs, restore closed tabs, go back and forth, open links in new tabs (No specific information about which websites to surf on yet)
2. At some point, tabs would not finish loading (or would take minutes to finish) and pressing the stop button does absolutely nothing.
3. The icon remains the X and the tab's spinner is still spinning

Actual behavior: Pages would never stop loading, even after pressing the stop button.
Expected behavior: Pages should stop loading on pressing stop button.

This is not consistently reproducible and we don't know for sure whether it is a regression or not.
 

Comment 1 Deleted

Comment 2 by cma...@chromium.org, Aug 16 2016

On tapping "Stop Loading" button, the icon does not change to "Reload". The icon remains the X and the tab's spinner is still spinning.
Steps to reproduce:
1. Load https://www.specialized.com/us/en/bikes/road/
2. Tap Stop before load is completed
Cc: pinkerton@chromium.org pkl@chromium.org rohitrao@chromium.org
WebView actually stops loading. Either embedder is not notified that the load was stopped or has UI bug.
Status: Started (was: Assigned)
This is M53 regression, which happened after ToolbarModelImplIOS::IsLoading refactoring.
Components: Mobile>WebView>Glue
Labels: -Type-Bug Type-Bug-Regression
Status: Fixed (was: Started)
Fixed here: https://codereview.chromium.org/2250563003/
Project Member

Comment 8 by bugdroid1@chromium.org, Aug 17 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/16e4de4de55c5c946b8bdfa47bfee274afc930f2

commit 16e4de4de55c5c946b8bdfa47bfee274afc930f2
Author: eugenebut <eugenebut@chromium.org>
Date: Wed Aug 17 00:09:15 2016

[ios] Set WebState::IsLoading flag to false if load is cancelled.

Before this change WebState::IsLoading was returning false which caused
Toolbar to think that page did not finish loading. In M52 and earlier
toolbar was relying to CRWWebController.loadPhase API to check the load
state so this bug is M53 regression.

Keeping WebState::IsLoading in sync with CRWWebController.loadPhase
fixes toobar bug and is generally a reasonable thing to do.

BUG= 638277 

Review-Url: https://codereview.chromium.org/2250563003
Cr-Commit-Position: refs/heads/master@{#412391}

[modify] https://crrev.com/16e4de4de55c5c946b8bdfa47bfee274afc930f2/ios/web/web_state/ui/crw_web_controller.mm

Issue 629351 has been merged into this issue.
The fix did not get into the latest Canary
Is there a unit test or EG test to ensure this behavior doesn't regress in the future?
There is no test for that. This is something that is tricky to test with our local http server. Claude added this to manual test cases (this is really easy to test manually, you just hit stop button before page finish its loading).

Functionality has regressed because WebState had a bug probably since we wrote this class. So if something will regress during the future refactoring (specifically moving from private web API to public web API), this will be a different thing.

I think we should automate things which are not hard to automate, and give us significant coverage (like Navigation tests which we are writing in Q3). Also I think that we should have better unit tests, but for that we need to refactor our WebController god class, which we have plans to do in the future quarters.

Writing tests for past regressions is great if they are easy to write, which is not the case for this bug. 

Hopefully it answers your question.

Is the local HTTP server asynchronous?  Can we write a test page that contains an IMG that never finishes loading?
Server is asynchronous, and writing test is possible but tricky. Taking into account the reason of this regression, I just don't feel that spending time on this specific test will pay back (especially taking into account that it has been added to manual test cases). If you strongly feel that we should test this I will write this test it's not a problem, I just feel that we can do different things to improve the quality (like refactor the code so we can finally write useful unit tests or write test suites for areas where regressions are very common).

Your questions suggested that we should go through all web regression bugs (we have the history of those in bugtracker) and identify which areas we should cover with EG tests (not hard to test, frequent regressions) and write those tests as a planned effort. And this is something that we can and probably should prioritize for Q4. WDYT?


There's no way to write a test that when we tell the webview to stop that the button is in the right state?
There was no canary since Aug 17. The test will be written. Per Comment #12 and #14 there is a way to write this test.
Labels: Merge-Request-53
Work fine in Chrome 54.0.2832.0 Canary.

Comment 18 by dimu@chromium.org, Aug 18 2016

Labels: -Merge-Request-53 Merge-Review-53 Hotlist-Merge-Review
[Automated comment] Less than 2 weeks to go before AppStore submit on M53, manual review required.
Status: Verified (was: Fixed)
Verified on 54.0.2832.0 canary iPad Air 2 iOS 9.3.2, iPad mini 9.3.3, iPad Pro iOS 9.2.1 
Page stop loading on pressing stop button (specialized.com)
Looks good. 

Labels: -Hotlist-Merge-review -Merge-Review-53 Merge-Approved-53
Project Member

Comment 21 by bugdroid1@chromium.org, Aug 18 2016

Labels: -merge-approved-53 merge-merged-2785
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/aa1c693955107d0e4d43ed59ebabe974ee698665

commit aa1c693955107d0e4d43ed59ebabe974ee698665
Author: Eugene But <eugenebut@google.com>
Date: Thu Aug 18 21:00:53 2016

[ios] Set WebState::IsLoading flag to false if load is cancelled.

Before this change WebState::IsLoading was returning false which caused
Toolbar to think that page did not finish loading. In M52 and earlier
toolbar was relying to CRWWebController.loadPhase API to check the load
state so this bug is M53 regression.

Keeping WebState::IsLoading in sync with CRWWebController.loadPhase
fixes toobar bug and is generally a reasonable thing to do.

BUG= 638277 

Review-Url: https://codereview.chromium.org/2250563003
Cr-Commit-Position: refs/heads/master@{#412391}
(cherry picked from commit 16e4de4de55c5c946b8bdfa47bfee274afc930f2)

Review URL: https://codereview.chromium.org/2260003002 .

Cr-Commit-Position: refs/branch-heads/2785@{#668}
Cr-Branched-From: 68623971be0cfc492a2cb0427d7f478e7b214c24-refs/heads/master@{#403382}

[modify] https://crrev.com/aa1c693955107d0e4d43ed59ebabe974ee698665/ios/web/web_state/ui/crw_web_controller.mm

Project Member

Comment 22 by bugdroid1@chromium.org, Aug 19 2016

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/chrome/ios_internal.git/+/2cc803f425524b0c974d6bfbcefc70f0c40661ce

commit 2cc803f425524b0c974d6bfbcefc70f0c40661ce
Author: eugenebut <eugenebut@google.com>
Date: Fri Aug 19 19:01:10 2016

Project Member

Comment 23 by bugdroid1@chromium.org, Aug 19 2016

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/chrome/ios_internal.git/+/2cc803f425524b0c974d6bfbcefc70f0c40661ce

commit 2cc803f425524b0c974d6bfbcefc70f0c40661ce
Author: eugenebut <eugenebut@google.com>
Date: Fri Aug 19 19:01:10 2016

Verified on 53.0.2785.80 beta iPad Air 2 iOS 10 beta 7, iPad Pro iOS 9.3.3, following the steps mentioned in comment # 0
Page stop loading on pressing stop button.  Looks good.

Sign in to add a comment