New issue
Advanced search Search tips

Issue 639787 link

Starred by 2 users

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Clarify expected behavior of BlobUrlOriginMap by comment and descriptive naming

Project Member Reported by tyoshino@chromium.org, Aug 22 2016

Issue description

https://chromium.googlesource.com/chromium/src/+/f4de7176ce6deb2bc02fa6b2187b226b9e13a91a%5E%21/ fixed WebKit to correctly handle unique / file origins by remembering map between Blob URLs created in unique / file origins and actual SecurityOrigin instances.

BlobUrlOriginMap is created as TLS by using the ThreadSpecific. jianli@, it's 4 years ago, so only in case you remember, tell me why it was made so, please.

It's named "cache" but it's not a cache in terms of that it must be valid everywhere the URL is used.
 
Labels: Needs-Feedback
tyoshino@ Is this just a question about the name? If not can you provide more detail on the bug?
I think the name is also need to be revised.

But the main question is why BlobUrlOriginMap is wrapped with ThreadSpecific to be TLS. I think it should not be thread-local.

Comment 3 by jsb...@chromium.org, Aug 29 2016

Being naive: what's the alternative? If it were shared by all threads we'd need a thread-safe map and SecurityOrigin itself is not ThreadSafeRefCounted (or internally thread-safe).

Comment 4 by mek@chromium.org, Aug 29 2016

Note that there has also been some discussion in the URL spec (https://github.com/whatwg/url/issues/127) about this BlobUrlOriginMap (as currently the whole map isn't specified at all).

Apparently this is an area where various browsers don't currently agree on, although I haven't looked as deeply into this as I should have.
Project Member

Comment 5 by bugdroid1@chromium.org, Sep 2 2016

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

commit d809019bba4fa9fb90265a64d311826ab3e356e2
Author: tyoshino <tyoshino@chromium.org>
Date: Fri Sep 02 06:19:56 2016

Rename SecurityOriginCache to URLSecurityOriginMap

SecurityOriginCache is needed to implement "the origin of the Blob URL"
defined in the File API spec. It's not a cache but a map which needs to
persist. Rename to less confusing name.

This CL also adds comments and do some refactoring around the logic for
readability.

R=mkwst@chromium.org,tommi@chromium.org
BUG=639787

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

[modify] https://crrev.com/d809019bba4fa9fb90265a64d311826ab3e356e2/third_party/WebKit/Source/core/dom/DOMURL.cpp
[modify] https://crrev.com/d809019bba4fa9fb90265a64d311826ab3e356e2/third_party/WebKit/Source/core/fileapi/Blob.cpp
[modify] https://crrev.com/d809019bba4fa9fb90265a64d311826ab3e356e2/third_party/WebKit/Source/core/html/PublicURLManager.cpp
[modify] https://crrev.com/d809019bba4fa9fb90265a64d311826ab3e356e2/third_party/WebKit/Source/core/html/PublicURLManager.h
[modify] https://crrev.com/d809019bba4fa9fb90265a64d311826ab3e356e2/third_party/WebKit/Source/modules/mediastream/URLMediaStream.idl
[modify] https://crrev.com/d809019bba4fa9fb90265a64d311826ab3e356e2/third_party/WebKit/Source/platform/BUILD.gn
[modify] https://crrev.com/d809019bba4fa9fb90265a64d311826ab3e356e2/third_party/WebKit/Source/platform/blob/BlobRegistry.cpp
[modify] https://crrev.com/d809019bba4fa9fb90265a64d311826ab3e356e2/third_party/WebKit/Source/platform/weborigin/SecurityOrigin.cpp
[modify] https://crrev.com/d809019bba4fa9fb90265a64d311826ab3e356e2/third_party/WebKit/Source/platform/weborigin/SecurityOrigin.h
[rename] https://crrev.com/d809019bba4fa9fb90265a64d311826ab3e356e2/third_party/WebKit/Source/platform/weborigin/URLSecurityOriginMap.h

Cc: -jianli@chromium.org
Owner: tyoshino@chromium.org
Status: Started (was: Untriaged)
Summary: Clarify expected behavior of BlobUrlOriginMap by comment and descriptive naming (was: Is it ok to use ThreadSpecific for BlobUrlOriginMap?)
jsbell@: I see. Fixing it would need some non trivial work.

mek@: Oh, thanks for the pointer! I understood the situation.

So, I'd leave some comment describing current situation so that people who get question are guided to the discussion.
Cc: toyoshim@chromium.org
Components: Blink>SecurityFeature Blink>Network
Labels: -Needs-Feedback
Owner: ----
+toyoshim@

This bug is related to the discussion we had last week. This is a part of the SecurityOrigin class and has been doing some special handling for Blob URLs.
Components: Blink>Storage>FileAPI
Components: -Blink>FileAPI
Status: Available (was: Started)
This issue has been marked as started, but has no owner. Making available.

Sign in to add a comment