Bug 63301 – Refactor ThreadableLoaderOptions and use it in ResourceLoader.

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

[Nate Chapin]

ThreadableLoaderOptions has nothing to do with threading.  It's also makes a relatively concise way to pass around a bunch of configuration bits for the loader, rather than passing separate bits for resource callbacks, content sniffing, etc.  Ergo, rename it, put it in its own file, and use it as a parameter down to ResourceLoader.

(This would also be required if we wanted to generalize access control enforcement and preflighting and move it out of DocumentThreadableLoader, which I'd like to do soon if folks agree it's a good idea.)

[Nate Chapin]

Created attachment 98430 [details]
patch

[WebKit Review Bot]

Attachment 98430 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/loader/ResourceLoadOptions.h:54:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/loader/ResourceLoadOptions.h:55:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 3 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 98430 [details]
patch

Attachment 98430 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8939158

[Nate Chapin]

Created attachment 98437 [details]
Address ews's comments

[Alexey Proskuryakov]

It's true that document resource loading (talking about images, stylesheets or scripts) is getting closer to programmatic loading (such as XMLHttpRequest or worker script loading, handled by ThreadableLoader). Some of the former will support CORS, while the latter will be cached in WebCore memory cache.

Having nothing to do with threading doesn't seem like a sound basis for this change though. The structure is a parameter block for ThreadableLoader, and making ThreadableLoader and ResourceLoader accept the same parameter block seems questionable before these classes themselves are merged into one.

[Adam Barth]

Comment on attachment 98437 [details]
Address ews's comments

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

> Source/WebCore/loader/ResourceLoadOptions.h:51
> +    ResourceLoadOptions() : sendLoadCallbacks(true), sniffContent(true), allowCredentials(true), forcePreflight(false), crossOriginRequestPolicy(AllowCrossOriginRequests), shouldBufferData(true) { }

This should probably be split into multiple lines.

[Adam Barth]

> It's true that document resource loading (talking about images, stylesheets or scripts) is getting closer to programmatic loading (such as XMLHttpRequest or worker script loading, handled by ThreadableLoader). Some of the former will support CORS, while the latter will be cached in WebCore memory cache.

Is there a particular reason to keep them separate?  There seems to be value in decreasing the number of paths through the loader.  I guess one difference is that ThreadableLoader has to worry about what's synchronous and what's asynchronous.

> Having nothing to do with threading doesn't seem like a sound basis for this change though. The structure is a parameter block for ThreadableLoader, and making ThreadableLoader and ResourceLoader accept the same parameter block seems questionable before these classes themselves are merged into one.

I think the goal is to merge these code paths in the future.  This seems like a positive step in that direction.  My only concern is callers setting options that aren't yet understood by the class they're using (before they're merged).  Maybe it would make sense to include some ASSERTs that as-yet unsupported options aren't used?

[Alexey Proskuryakov]

I do not see how this step is necessary at this stage. We don't even know for sure yet if we'll merge the loader classes, but we're already merging their parameter blocks, which have some parameters that are not applicable to both.

[Nate Chapin]

Created attachment 98504 [details]
WIP for generalizing preflighting

Attached is a mostly working but hacky patch that does the ThreadableLoaderOptions rename and moves preflighting into a separate class attached to ResourceLoader.   Hopefully this gives a little more context into where I was thinking of going with this.

[Alexey Proskuryakov]

It would be nice to get comments from folks who actively work on ResourceLoader, as well as on ThreadableLoader. Some of them are CC'ed here, please do ask anyone else who may have an opinion.

[David Levin]

Comment on attachment 98437 [details]
Address ews's comments

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

At a meta level, I wonder a few things:

1. Why do we need to move down the preflight stuff into ResourceLoader (which adds more complexity at that level)? Does everything using ResourceLoader need them? Can we people who need preflight to using DocumentThreadableLoader (and perhaps rename *ThreadableLoader to *PreflightLoader or *CORSLoaders)?

2. Why not create a new struct ResourceLoaderOptions and derive ThreadableLoaderOptions from that? Only move the fields into ResourceLoaderOptions which actually do something. (With the current patch I can fill in fields that do nothing which would be confusing.)

> Source/WebCore/notifications/Notification.cpp:172
> +    options.allowCredentials = true;

This is flipping the value from false to true. While this looks correct, it would be good to do this as a separate bug instead of putting it in as a part of this bigger change.

> Source/WebCore/loader/ResourceLoadOptions.h:50
> +struct ResourceLoadOptions {

Seems like this should be ResourceLoad*er*Options.

>> Source/WebCore/loader/ResourceLoadOptions.h:51
>> +    ResourceLoadOptions() : sendLoadCallbacks(true), sniffContent(true), allowCredentials(true), forcePreflight(false), crossOriginRequestPolicy(AllowCrossOriginRequests), shouldBufferData(true) { }
> 
> This should probably be split into multiple lines.

Changing the defaults from restrictive/conservations options to less conservative options makes it too easy to introduce unintended security issues.

From example:
  allowCredentials from false to true.
  crossOriginRequestPolicy from DenyCrossOriginRequests to AllowCrossOriginRequests
etc.

Please change all of these back to what they were.  (Also this is addressing a different issues from "Rename ThreadableLoaderOptions and use it down to ResourceLoader").

> Source/WebCore/loader/ThreadableLoader.h:-48
> -    enum StoredCredentials {

This placement is an artifact of history, so it shouldn't move to the file with ThreadableLoaderOptions but it should move elsewhere (possibly Source/WebCore/platform/network/ResourceHandle.h).

Regardless this is a separate issue from the one being addressed in this patch (of moving ThreadableLoaderOptions).

[Nate Chapin]

(In reply to comment #11)
> (From update of attachment 98437 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=98437&action=review
> 
> At a meta level, I wonder a few things:
> 
> 1. Why do we need to move down the preflight stuff into ResourceLoader (which adds more complexity at that level)? Does everything using ResourceLoader need them? Can we people who need preflight to using DocumentThreadableLoader (and perhaps rename *ThreadableLoader to *PreflightLoader or *CORSLoaders)?

Given the stated goal of https://bugs.webkit.org/show_bug.cgi?id=61225, I thought that it would make more sense to have the knowledge of CORS be at or below the cache layer, rather than making a user of the cache have to handle it.  I don't think it necessarily needs to be at ResourceLoader, but I guess it mostly depends whether we think CORS preflighting will be used in more places in the future.

> 
> 2. Why not create a new struct ResourceLoaderOptions and derive ThreadableLoaderOptions from that? Only move the fields into ResourceLoaderOptions which actually do something. (With the current patch I can fill in fields that do nothing which would be confusing.)

I had thought about that, but in the WIP I posted, all of ResourceLoaderOptions' fields are used at the ResourceLoader level.  I was thinking it didn't make sense to do subclassing in the interim, but I don't feel strongly about that.

> 
> > Source/WebCore/notifications/Notification.cpp:172
> > +    options.allowCredentials = true;
> 
> This is flipping the value from false to true. While this looks correct, it would be good to do this as a separate bug instead of putting it in as a part of this bigger change.
> 
> > Source/WebCore/loader/ResourceLoadOptions.h:50
> > +struct ResourceLoadOptions {
> 
> Seems like this should be ResourceLoad*er*Options.
> 
> >> Source/WebCore/loader/ResourceLoadOptions.h:51
> >> +    ResourceLoadOptions() : sendLoadCallbacks(true), sniffContent(true), allowCredentials(true), forcePreflight(false), crossOriginRequestPolicy(AllowCrossOriginRequests), shouldBufferData(true) { }
> > 
> > This should probably be split into multiple lines.
> 
> Changing the defaults from restrictive/conservations options to less conservative options makes it too easy to introduce unintended security issues.

Yeah, I had changed the defaults to what they are at the ResourceLoader level.  You're probably right that that's a risky idea.

> 
> From example:
>   allowCredentials from false to true.
>   crossOriginRequestPolicy from DenyCrossOriginRequests to AllowCrossOriginRequests
> etc.
> 
> Please change all of these back to what they were.  (Also this is addressing a different issues from "Rename ThreadableLoaderOptions and use it down to ResourceLoader").
> 
> > Source/WebCore/loader/ThreadableLoader.h:-48
> > -    enum StoredCredentials {
> 
> This placement is an artifact of history, so it shouldn't move to the file with ThreadableLoaderOptions but it should move elsewhere (possibly Source/WebCore/platform/network/ResourceHandle.h).
> 
> Regardless this is a separate issue from the one being addressed in this patch (of moving ThreadableLoaderOptions).

[Nate Chapin]

Created attachment 105093 [details]
Addressing levin's comments

[WebKit Review Bot]

Comment on attachment 105093 [details]
Addressing levin's comments

Attachment 105093 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9510057

[David Levin]

Comment on attachment 105093 [details]
Addressing levin's comments

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

Just a few issues.

> Source/WebCore/loader/ResourceLoaderOptions.h:44
> +const ResourceLoaderOptions& defaultResourceOptions();

I find it confusing that we have some called defaultResourceOptions which is different from what the default constructor does.

What makes this more "default" than the "default" constructor?

Ultimately, what I'm getting at is that I think this is a bad name (and I don't know what a better name is because I don't understand it).

> Source/WebCore/loader/ResourceLoader.h:183
> +        ResourceLoaderOptions m_options;

What does it meant that ResourceLoaderOptions has this extra field that wasn't in ResourceLoader before?
    bool allowCredentials; // Whether HTTP credentials and cookies are sent with the request.

It seems odd to allow a new option to be passed in which isn't supported.

> Source/WebCore/loader/ThreadableLoader.h:67
> +        ThreadableLoaderOptions() : ResourceLoaderOptions(), preflightPolicy(ConsiderPreflight), crossOriginRequestPolicy(DenyCrossOriginRequests) { }

I'm pretty sure you don't need to list "ResourceLoaderOptions()" here.

> Source/WebCore/loader/ResourceLoaderOptions.cpp:38
> +    static bool s_optionsInitialized = false;

This isn't thread-safe. Please add an ASSERT(isMainThread()); here.

[Nate Chapin]

> > Source/WebCore/loader/ResourceLoaderOptions.h:44
> > +const ResourceLoaderOptions& defaultResourceOptions();
> 
> I find it confusing that we have some called defaultResourceOptions which is different from what the default constructor does.

Yeah....I wasn't quite sure how to handle this case where we have to deal with multiple different defaults.

So I think my options are:
(1) Rename defaultResourceOptions()
(2) Add a constructor that takes arguments for all the variables.
(3) Make ResourceLoaderOptions' initial values be the ones in defaultResourceOptions(), and make the current initial values be overrides set by ThreadableLoaderOptions.
(4) Have several different classes have their own equivalent of defaultResourceOptions()

I'm leaning toward (2).

> > Source/WebCore/loader/ResourceLoader.h:183
> > +        ResourceLoaderOptions m_options;
> 
> What does it meant that ResourceLoaderOptions has this extra field that wasn't in ResourceLoader before?
>     bool allowCredentials; // Whether HTTP credentials and cookies are sent with the request.
> 
> It seems odd to allow a new option to be passed in which isn't supported.

Doh, I moved a variable that wasn't in use yet (again). Will move it back to ThreadableLoaderOptions.

[Nate Chapin]

Created attachment 105202 [details]
2 constructors, allowCredentials back to ThreadableLoaderOptions

[David Levin]

Comment on attachment 105202 [details]
2 constructors, allowCredentials back to ThreadableLoaderOptions

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

> Source/WebCore/loader/ResourceLoaderOptions.h:38
> +    ResourceLoaderOptions(bool sendLoadCallbacksArg, bool sniffContentArg, bool shouldBufferDataArg) : sendLoadCallbacks(sendLoadCallbacksArg), sniffContent(sniffContentArg), shouldBufferData(shouldBufferDataArg) { }

I loathe this -- 3 bool arguments in a row. It makes all call sites impossible to read.

What does this do (without looking above):
  ResourceLoaderOptions(true, false, true);

Anyway this is basically what was there before so ok. If you add more bools in here, you should consider making them enums. (Even in a future patch, converting these to enums would be awesome.)

[Nate Chapin]

(In reply to comment #18)
> (From update of attachment 105202 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=105202&action=review
> 
> > Source/WebCore/loader/ResourceLoaderOptions.h:38
> > +    ResourceLoaderOptions(bool sendLoadCallbacksArg, bool sniffContentArg, bool shouldBufferDataArg) : sendLoadCallbacks(sendLoadCallbacksArg), sniffContent(sniffContentArg), shouldBufferData(shouldBufferDataArg) { }
> 
> I loathe this -- 3 bool arguments in a row. It makes all call sites impossible to read.
> 
> What does this do (without looking above):
>   ResourceLoaderOptions(true, false, true);
> 
> Anyway this is basically what was there before so ok. If you add more bools in here, you should consider making them enums. (Even in a future patch, converting these to enums would be awesome.)

Ok, I'll enum-ify these next.

[WebKit Review Bot]

Comment on attachment 105202 [details]
2 constructors, allowCredentials back to ThreadableLoaderOptions

Clearing flags on attachment: 105202

Committed r93811: <http://trac.webkit.org/changeset/93811>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.