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

Issue 758560 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Nov 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 1
Type: Task



Sign in to add a comment

Writing to disk should not be done on the main thread.

Project Member Reported by jif@chromium.org, Aug 24 2017

Issue description

In ntp_tile_saver.mm, the following code (writeToURL:) writes to disks from the main thread.

void (^faviconImageBlock)(UIImage*) = ^(UIImage* favicon) {
      tile.faviconFetched = YES;
      NSData* imageData = UIImagePNGRepresentation(favicon);
      if ([imageData writeToURL:fileURL atomically:YES]) {
        tile.faviconFileName = faviconFileName;
      }
      WriteToDiskIfComplete(tiles, faviconsURL);
    };

There's an other place in related code where that happens, but I can't find it anymore.
 
Components: -Internals UI>Browser>NewTabPage
Labels: -Type-Bug Type-Task

Comment 2 by fi...@chromium.org, Sep 4 2017

Labels: zine-triaged
Components: -UI>Browser>NewTabPage UI>Browser>ContentSuggestions
Labels: -Pri-3 M-63 Pri-1
Writing to disk on UI thread can freeze the UI. Can we prioritize the fix?

Comment 4 by lod@chromium.org, Oct 5 2017

Status: Started (was: Assigned)
Project Member

Comment 5 by bugdroid1@chromium.org, Oct 20 2017

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

commit 7f0472d85a62efebe0551537fe80f1f0794af6d4
Author: Elodie Banel <lod@google.com>
Date: Fri Oct 20 22:10:49 2017

Save NTP tiles offline on the IO thread

Bug:  758560 
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: Id40ef2e9be3cdc81da0e04f72c48d7f2e40e0213
Reviewed-on: https://chromium-review.googlesource.com/702308
Commit-Queue: Elodie Banel <lod@chromium.org>
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#510578}
[modify] https://crrev.com/7f0472d85a62efebe0551537fe80f1f0794af6d4/ios/chrome/browser/ui/ntp/ntp_tile_saver.h
[modify] https://crrev.com/7f0472d85a62efebe0551537fe80f1f0794af6d4/ios/chrome/browser/ui/ntp/ntp_tile_saver.mm
[modify] https://crrev.com/7f0472d85a62efebe0551537fe80f1f0794af6d4/ios/chrome/browser/ui/ntp/ntp_tile_saver_unittest.mm

Project Member

Comment 6 by bugdroid1@chromium.org, Oct 23 2017

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

commit 9a494a4e49188d2870df05b3e94c665743d984ff
Author: Gauthier Ambard <gambard@chromium.org>
Date: Mon Oct 23 15:22:06 2017

Revert "Save NTP tiles offline on the IO thread"

This reverts commit 7f0472d85a62efebe0551537fe80f1f0794af6d4.

Reason for revert: Introduce a crash on device on fresh install.

Original change's description:
> Save NTP tiles offline on the IO thread
> 
> Bug:  758560 
> Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet
> Change-Id: Id40ef2e9be3cdc81da0e04f72c48d7f2e40e0213
> Reviewed-on: https://chromium-review.googlesource.com/702308
> Commit-Queue: Elodie Banel <lod@chromium.org>
> Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#510578}

TBR=rohitrao@chromium.org,sdefresne@chromium.org,lod@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  758560 
Change-Id: I0d0fec14287ef146d985c862165648aa27030cc2
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet
Reviewed-on: https://chromium-review.googlesource.com/732819
Reviewed-by: Gauthier Ambard <gambard@chromium.org>
Commit-Queue: Gauthier Ambard <gambard@chromium.org>
Cr-Commit-Position: refs/heads/master@{#510799}
[modify] https://crrev.com/9a494a4e49188d2870df05b3e94c665743d984ff/ios/chrome/browser/ui/ntp/ntp_tile_saver.h
[modify] https://crrev.com/9a494a4e49188d2870df05b3e94c665743d984ff/ios/chrome/browser/ui/ntp/ntp_tile_saver.mm
[modify] https://crrev.com/9a494a4e49188d2870df05b3e94c665743d984ff/ios/chrome/browser/ui/ntp/ntp_tile_saver_unittest.mm

Project Member

Comment 7 by bugdroid1@chromium.org, Oct 27 2017

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

commit a6e2558f8955adadae4b373526d2c010f5999e71
Author: Elodie Banel <lod@google.com>
Date: Fri Oct 27 14:59:44 2017

Reland save NTP tiles offline off the main thread

Was "Save NTP tiles offline on the IO thread". Reverted for crashing
on new install.
Issue (afaict...) was that most_visited_data is a reference and not
retained by BindBlockArc, so I bind it manually.

Bug:  758560 
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: Id79b3b8c7f2c7b6973f2d4725ee056775d410916
Reviewed-on: https://chromium-review.googlesource.com/738091
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Commit-Queue: Elodie Banel <lod@chromium.org>
Cr-Commit-Position: refs/heads/master@{#512183}
[modify] https://crrev.com/a6e2558f8955adadae4b373526d2c010f5999e71/ios/chrome/browser/ui/ntp/ntp_tile_saver.h
[modify] https://crrev.com/a6e2558f8955adadae4b373526d2c010f5999e71/ios/chrome/browser/ui/ntp/ntp_tile_saver.mm
[modify] https://crrev.com/a6e2558f8955adadae4b373526d2c010f5999e71/ios/chrome/browser/ui/ntp/ntp_tile_saver_unittest.mm

Comment 8 by lod@chromium.org, Nov 6 2017

Status: Fixed (was: Started)

Sign in to add a comment