New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 695493 link

Starred by 2 users

Issue metadata

Status: Verified
Owner:
Closed: Feb 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 2
Type: Bug



Sign in to add a comment

Crash in reading_list::ReadingListDistillerPage::IsGoogleCachedAMPPage()

Project Member Reported by olivierrobin@chromium.org, Feb 23 2017

Issue description


crash id 5c57b9d300000000

1 report in M57 - M58

	0x0000000100234694	(Chrome -reading_list_distiller_page.mm:189 )	reading_list::ReadingListDistillerPage::IsGoogleCachedAMPPage()
0x0000000100234690	(Chrome -reading_list_distiller_page.mm:189 )	reading_list::ReadingListDistillerPage::IsGoogleCachedAMPPage()
0x0000000100234648	(Chrome -reading_list_distiller_page.mm:166 )	reading_list::ReadingListDistillerPage::DelayedOnLoadURLDone()
0x00000001006d82d4	(Chrome -callback.h:68 )	base::debug::TaskAnnotator::RunTask(char const*, base::PendingTask*)
0x00000001006ed038	(Chrome -message_loop.cc:423 )	base::MessageLoop::RunTask(base::PendingTask*)
0x00000001006ed284	(Chrome -message_loop.cc:434 )	base::MessageLoop::DeferOrRunPendingTask(base::PendingTask)
0x00000001006ed754	(Chrome -message_loop.cc:566 )	base::MessageLoop::DoDelayedWork(base::TimeTicks*)
0x0000000100740934	(Chrome -message_pump_mac.mm:306 )	base::MessagePumpCFRunLoopBase::RunWork()
 
Project Member

Comment 1 by bugdroid1@chromium.org, Feb 23 2017

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

commit d7e66510bedbf95d90337071155f8b07036cb002
Author: olivierrobin <olivierrobin@chromium.org>
Date: Thu Feb 23 17:43:33 2017

[Reading List] Check web state after the distillation delay.

It is possible that the web state has been released before the timeout
of the timer.
If this is the case, it means that the distillation was aborted.
Early return in that case.

There is no Cancelable delayed task. As the current situation is simple (only one
task is running at a time), store the ID of the task, and increment it when cancelling.
This is inspired by RunIfNotCanceled, but adapted to this much simpler situation.
Check the current ID when the task runs. If the ID was incremented, the task is cancelled.

BUG= 695493 

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

[modify] https://crrev.com/d7e66510bedbf95d90337071155f8b07036cb002/ios/chrome/browser/reading_list/reading_list_distiller_page.h
[modify] https://crrev.com/d7e66510bedbf95d90337071155f8b07036cb002/ios/chrome/browser/reading_list/reading_list_distiller_page.mm

Cc: cma...@chromium.org
Labels: ReleaseBlock-Stable M-57 Merge-Request-57
Status: Fixed (was: Started)
I investigated more.
This can happen if the webkit process is killed during the rendering delay.
So I think it would be better to cherry-pick the fix.
Project Member

Comment 3 by sheriffbot@chromium.org, Feb 24 2017

Labels: -Merge-Request-57 Hotlist-Merge-Review Merge-Review-57
This bug requires manual review: Less than 14 days to go before AppStore submit on M57
Please contact the milestone owner if you have questions.
Owners: amineer@(clank), cmasso@(bling), ketakid@(cros), govind@(desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Ping cmasso

Comment 5 by cma...@chromium.org, Feb 27 2017

Labels: -Hotlist-Merge-Review -Merge-Review-57 Merge-Approved-57
Issue 696352 has been merged into this issue.
Project Member

Comment 7 by bugdroid1@chromium.org, Feb 28 2017

Labels: -merge-approved-57 merge-merged-2987
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ffaa9b73b76d5c15755975266f94948413b7389a

commit ffaa9b73b76d5c15755975266f94948413b7389a
Author: Olivier Robin <olivierrobin@chromium.org>
Date: Tue Feb 28 09:32:33 2017

[Reading List] Check web state after the distillation delay.

It is possible that the web state has been released before the timeout
of the timer.
If this is the case, it means that the distillation was aborted.
Early return in that case.

There is no Cancelable delayed task. As the current situation is simple (only one
task is running at a time), store the ID of the task, and increment it when cancelling.
This is inspired by RunIfNotCanceled, but adapted to this much simpler situation.
Check the current ID when the task runs. If the ID was incremented, the task is cancelled.

BUG= 695493 

Review-Url: https://codereview.chromium.org/2717483003
Cr-Commit-Position: refs/heads/master@{#452540}
(cherry picked from commit d7e66510bedbf95d90337071155f8b07036cb002)

Review-Url: https://codereview.chromium.org/2724583002 .
Cr-Commit-Position: refs/branch-heads/2987@{#713}
Cr-Branched-From: ad51088c0e8776e8dcd963dbe752c4035ba6dab6-refs/heads/master@{#444943}

[modify] https://crrev.com/ffaa9b73b76d5c15755975266f94948413b7389a/ios/chrome/browser/reading_list/reading_list_distiller_page.h
[modify] https://crrev.com/ffaa9b73b76d5c15755975266f94948413b7389a/ios/chrome/browser/reading_list/reading_list_distiller_page.mm

Status: Verified (was: Fixed)
Verified in 58.0.3026.0 canary, iPhone 6+ iOS 10.2
No crashes are seen when distillation is delayed.
Steps:
1. Sign in
2. Add entry and immediately turn wifi off.
3. Turn on wifi
No crashes are seen when distillation is delayed.
Looks good.
Verified the issue on the latest beta 57.0.2987.95, tested on iphone 7+(iOS10).
No crash is seen with the steps mentioned in comment#8, works fine

Sign in to add a comment