New issue
Advanced search Search tips
Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Launch-OWP
Launch-Accessibility: ----
Launch-Exp-Leadership: ----
Launch-Leadership: ----
Launch-Legal: ----
Launch-M-Approved: ----
Launch-M-Target: ----
Launch-Privacy: ----
Launch-Security: ----
Launch-Test: ----
Launch-UI: ----



Sign in to add a comment
link

Issue 688586: FileReaderSync should not be exposed in service workers

Reported by mek@chromium.org, Feb 3 2017 Project Member

Issue description

As discussed in https://github.com/w3c/ServiceWorker/issues/735 and https://github.com/w3c/FileAPI/issues/16, and changed in the spec in https://github.com/w3c/FileAPI/commit/6a24b10486aca75a85b95c979ae362f130a69523 the FileReaderSync interface should not be exposed in service workers.

No idea how much actual usage that API might have in service workers though. Both firefox and chrome are currently exposing it as far as I can tell.
 

Comment 1 by pwnall@chromium.org, Feb 5 2017

Cc: pwnall@chromium.org
Labels: -Pri-3 OS-All Pri-1
Status: Available (was: Untriaged)
Would you like to own the deprecation and removal of the feature?

I think we should act sooner rather than later, to make sure this doesn't become used. Hence making it high priority. If you don't want the bug, I can take it or find someone who does.

Comment 2 by falken@chromium.org, Feb 6 2017

If you want to get usage statistics,  issue 376039  tracks adding use counter support from service workers.

Comment 3 by nhiroki@chromium.org, Feb 6 2017

Cc: rbyers@chromium.org
Yes, UseCounter for Service Worker will be available maybe soon (in Q1?). However, UseCounter doesn't record what kind of GlobalScope uses the feature, so it wouldn't be so useful in this case where we want to know usage only on ServiceWorkerGlobalScope...

(+rbyers@ to CC for an interesting (rare?) use case of UseCounter)

Comment 4 by jsb...@chromium.org, Feb 6 2017

Off on a tangent, but I just got a ping from Microsoft wondering about collecting stats on a particular Indexed DB case, distinguishing SW from non-SW use.

Comment 5 by nhiroki@chromium.org, Feb 8 2017

We could add a UseCounter::Feature only for ServiceWorker like RequestFileSystemWorker. This way is not scalable but feasible after SW UseCounter is enabled.

Comment 6 by mek@chromium.org, Feb 8 2017

Yeah, having use counters split out by global type might be nice to have in general, but for individual cases it's not that hard to add custom use counters for specific global types. So I guess the question here is where the balance between waiting longer to delete this so we can collect data on usage, vs just deleting it sooner to avoid more usage should be.

Comment 7 by pwnall@chromium.org, Feb 8 2017

IMHO we should deprecate FileReaderSync immediately, to mitigate the risk of adoption, and remove as soon as we can.

Comment 8 by nhiroki@chromium.org, Feb 8 2017

Either way is fine with me. I'm assuming the rate of FileReaderSync use on ServiceWorker is quite low.

Comment 9 by mek@chromium.org, Feb 8 2017

Yeah, I'm assuming the same. Seems two sensible options would be:
1) remove completely in M58, hoping nothing breaks, or
2) deprecate in M58, hopefully usecounters will also land in M58, and then unless usage turns out to be too high remove in M59

Comment 10 by mek@chromium.org, Feb 8 2017

Labels: -Type-Bug OWP-Design-No OWP-Standards-UnofficialSpec OWP-Type-Deprecation Type-Launch-OWP
Owner: mek@chromium.org
Status: Started (was: Available)

Comment 11 by pwnall@chromium.org, Feb 8 2017

How about deprecating in M58, and attempting to merge the deprecation to M57?

Comment 12 by mek@chromium.org, Feb 8 2017

Merging the deprecation warning to M57 might make sense, yes. I don't think there is any hope to get use counters for service workers merged to M57 as well though, but just having the warning would still be nice of course.

I started drafting an intent-to-deprecate at https://docs.google.com/document/d/1xHi38EhijVnb8HCbk1MN1MLbeGnKq6gfd5PWSk6MvHo/edit, comments welcome

Comment 14 by mek@chromium.org, Feb 13 2017

Labels: Merge-Request-57
I'd like to merge the UMA change from comment #13 to M57 in order to collect data sooner.

Comment 15 by sheriffbot@chromium.org, Feb 13 2017

Project Member
Labels: -Merge-Request-57 Hotlist-Merge-Approved Merge-Approved-57
Your change meets the bar and is auto-approved for M57. Please go ahead and merge the CL to branch 2987 manually. Please contact 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

Comment 16 by bugdroid1@chromium.org, Feb 13 2017

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

commit f9a5a3f987c9c5c4af122effef4440d97838e568
Author: Marijn Kruisselbrink <mek@chromium.org>
Date: Mon Feb 13 21:57:39 2017

Add a use counter and histogram to FileReaderSync.

To hopefully eventually facilitate removing FileReaderSync from service
workers this adds both a use counter and histogram to log how often the
API is used in the various worker types.

BUG= 688586 

Review-Url: https://codereview.chromium.org/2682223006
Cr-Commit-Position: refs/heads/master@{#449833}
(cherry picked from commit 77f740e334ba6fdb4d6493372e8009c544939ed4)

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

[modify] https://crrev.com/f9a5a3f987c9c5c4af122effef4440d97838e568/third_party/WebKit/Source/core/fileapi/FileReaderSync.cpp
[modify] https://crrev.com/f9a5a3f987c9c5c4af122effef4440d97838e568/third_party/WebKit/Source/core/fileapi/FileReaderSync.h
[modify] https://crrev.com/f9a5a3f987c9c5c4af122effef4440d97838e568/third_party/WebKit/Source/core/fileapi/FileReaderSync.idl
[modify] https://crrev.com/f9a5a3f987c9c5c4af122effef4440d97838e568/third_party/WebKit/Source/core/frame/UseCounter.h
[modify] https://crrev.com/f9a5a3f987c9c5c4af122effef4440d97838e568/tools/metrics/histograms/histograms.xml

Comment 18 by mek@chromium.org, Feb 16 2017

Labels: Merge-Request-57
I'd also like to backport the change in commend #17 to M57

Comment 19 by sheriffbot@chromium.org, Feb 16 2017

Project Member
Labels: -Merge-Request-57 Merge-Approved-57
Your change meets the bar and is auto-approved for M57. Please go ahead and merge the CL to branch 2987 manually. Please contact 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

Comment 21 by bugdroid1@chromium.org, Mar 21 2017

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2a0319f1e13c5d4f7e8eb7771a8818f31a951b01

commit 2a0319f1e13c5d4f7e8eb7771a8818f31a951b01
Author: mek <mek@chromium.org>
Date: Tue Mar 21 18:40:00 2017

Disable FileReaderSync in service workers.

Usage so far in service workers is zero, so go ahead with disabling
FileReaderSync in service workers as planned.

Intent to deprecate and remove:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/cjWtqRD6iw8/hcxtAeE2CAAJ

BUG= 688586 

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

[modify] https://crrev.com/2a0319f1e13c5d4f7e8eb7771a8818f31a951b01/third_party/WebKit/LayoutTests/external/wpt/FileAPI/historical.https-expected.txt
[modify] https://crrev.com/2a0319f1e13c5d4f7e8eb7771a8818f31a951b01/third_party/WebKit/LayoutTests/http/tests/serviceworker/webexposed/global-interface-listing-service-worker-expected.txt
[modify] https://crrev.com/2a0319f1e13c5d4f7e8eb7771a8818f31a951b01/third_party/WebKit/LayoutTests/virtual/service-worker-navigation-preload/http/tests/serviceworker/webexposed/global-interface-listing-service-worker-expected.txt
[modify] https://crrev.com/2a0319f1e13c5d4f7e8eb7771a8818f31a951b01/third_party/WebKit/LayoutTests/virtual/stable/http/tests/serviceworker/webexposed/global-interface-listing-service-worker-expected.txt
[modify] https://crrev.com/2a0319f1e13c5d4f7e8eb7771a8818f31a951b01/third_party/WebKit/Source/core/fileapi/FileReaderSync.cpp
[modify] https://crrev.com/2a0319f1e13c5d4f7e8eb7771a8818f31a951b01/third_party/WebKit/Source/core/fileapi/FileReaderSync.idl
[modify] https://crrev.com/2a0319f1e13c5d4f7e8eb7771a8818f31a951b01/third_party/WebKit/Source/core/frame/Deprecation.cpp
[modify] https://crrev.com/2a0319f1e13c5d4f7e8eb7771a8818f31a951b01/third_party/WebKit/Source/core/frame/UseCounter.h

Comment 22 by mek@chromium.org, Mar 28 2017

Labels: M-59
Status: Fixed (was: Started)

Comment 23 by benhenry@chromium.org, Jun 15 2018

Components: Blink>Storage>FileAPI

Comment 24 by benhenry@chromium.org, Jun 15 2018

Components: -Blink>FileAPI

Sign in to add a comment