Bug 162118 - Cleanup: Remove an extraneous copy of SecurityOrigin
Summary: Cleanup: Remove an extraneous copy of SecurityOrigin
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-09-17 07:55 PDT by Daniel Bates
Modified: 2016-09-21 16:20 PDT (History)
7 users (show)

See Also:


Attachments
Patch (3.00 KB, patch)
2016-09-17 08:00 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch (3.00 KB, patch)
2016-09-17 08:09 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch (3.17 KB, patch)
2016-09-17 08:23 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch (3.45 KB, patch)
2016-09-17 08:31 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch (3.51 KB, patch)
2016-09-17 11:07 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2016-09-17 07:55:39 PDT
In WorkerThreadableLoader::MainThreadBridge::MainThreadBridge it is sufficient to make one isolated copy of SecurityOrigin that can be passed to the ContentSecurityPolicy and LoaderTaskOptions constructors.
Comment 1 Daniel Bates 2016-09-17 08:00:59 PDT
Created attachment 289165 [details]
Patch
Comment 2 Daniel Bates 2016-09-17 08:09:33 PDT
Created attachment 289166 [details]
Patch
Comment 3 Daniel Bates 2016-09-17 08:23:05 PDT
Created attachment 289167 [details]
Patch
Comment 4 Daniel Bates 2016-09-17 08:31:04 PDT
Created attachment 289169 [details]
Patch
Comment 5 youenn fablet 2016-09-17 08:41:02 PDT
Comment on attachment 289169 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=289169&action=review

> Source/WebCore/loader/WorkerThreadableLoader.cpp:115
> +    auto optionsCopy = std::make_unique<LoaderTaskOptions>(options, request.httpReferrer().isNull() ? outgoingReferrer : request.httpReferrer(), securityOriginCopy.ptr());

Can securityOriginCopy be moved?
Comment 6 Daniel Bates 2016-09-17 11:07:56 PDT
Created attachment 289173 [details]
Patch
Comment 7 Daniel Bates 2016-09-19 15:36:08 PDT
Comment on attachment 289173 [details]
Patch

Clearing flags on attachment: 289173

Committed r206122: <http://trac.webkit.org/changeset/206122>
Comment 8 Daniel Bates 2016-09-19 15:36:13 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Darin Adler 2016-09-20 11:30:46 PDT
Comment on attachment 289173 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=289173&action=review

> Source/WebCore/loader/WorkerThreadableLoader.cpp:88
> +    LoaderTaskOptions(const ThreadableLoaderOptions&, const String&, Ref<SecurityOrigin>&&);

Why do we need this constructor?
Comment 10 Daniel Bates 2016-09-21 14:10:51 PDT
Comment on attachment 289173 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=289173&action=review

>> Source/WebCore/loader/WorkerThreadableLoader.cpp:88
>> +    LoaderTaskOptions(const ThreadableLoaderOptions&, const String&, Ref<SecurityOrigin>&&);
> 
> Why do we need this constructor?

I take it you feel that it would make the code more understandable to use the default constructor and have the caller instantiate a LoaderTaskOptions with isolated copies. Currently, this constructor makes isolated copies of all its arguments except SecurityOrigin.
Comment 11 Darin Adler 2016-09-21 16:20:11 PDT
Comment on attachment 289173 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=289173&action=review

>>> Source/WebCore/loader/WorkerThreadableLoader.cpp:88
>>> +    LoaderTaskOptions(const ThreadableLoaderOptions&, const String&, Ref<SecurityOrigin>&&);
>> 
>> Why do we need this constructor?
> 
> I take it you feel that it would make the code more understandable to use the default constructor and have the caller instantiate a LoaderTaskOptions with isolated copies. Currently, this constructor makes isolated copies of all its arguments except SecurityOrigin.

OK, did not realize this constructor made isolated copies. I think that’s a bit subtle, so the question is what is a clearer idiom for that. For most classes, constructing makes a non-isolated copy and there is another function to make an isolated copy. If it was me I would:

1) Remove this constructor.
2) This effectively leaves us with a constructor that does copy or move for each data member (member-wise initialization).
3) Change each call site where a LoaderTaskOptions is created to move each piece in if the object is already isolated, or make an isolatedCopy as needed.

But maybe that will make the code harder to get right. Not sure.

It’s particularly unclear that this takes ownership of a SecurityOrigin, but makes isolated copies of the other things. What guarantees that the SecurityOrigin is already isolated with no other reference held to it? Seems easy to accidentally use that part wrong on a SecurityOrigin that is not isolated.