Bug 162118 – Cleanup: Remove an extraneous copy of SecurityOrigin

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Daniel Bates]

In WorkerThreadableLoader::MainThreadBridge::MainThreadBridge it is sufficient to make one isolated copy of SecurityOrigin that can be passed to the ContentSecurityPolicy and LoaderTaskOptions constructors.

[Daniel Bates]

Created attachment 289165 [details]
Patch

[Daniel Bates]

Created attachment 289166 [details]
Patch

[Daniel Bates]

Created attachment 289167 [details]
Patch

[Daniel Bates]

Created attachment 289169 [details]
Patch

[youenn fablet]

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?

[Daniel Bates]

Created attachment 289173 [details]
Patch

[Daniel Bates]

Comment on attachment 289173 [details]
Patch

Clearing flags on attachment: 289173

Committed r206122: <http://trac.webkit.org/changeset/206122>

[Daniel Bates]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

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?

[Daniel Bates]

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.

[Darin Adler]

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.