Removal of rel=preload link elements should stop the download.

Removal of rel=preload link elements should stop the download.

From the spec: https://w3c.github.io/preload/
"The user agent should abort the request if the href attribute on the link element of an preload link is removed, or its value is set to an empty string."

This CL aligns the implementation with that part of the spec.

BUG=489646
Committed: https://crrev.com/4548d907a4d13c34bc290c77753c30cf9adaceda
Cr-Commit-Position: refs/heads/master@{#370909}

[Yoav Weiss, 2016-01-21 10:54:10]

yoav@yoav.ws changed reviewers:
+ japhet@chromium.org

[Yoav Weiss, 2016-01-21 10:54:10]

Hey Nate. Got another one for you :)

[Nate Chapin, 2016-01-21 19:03:50]

I'd suggested changing the title to "Removal of rel=preload link elements should
stop the download." I was confused as to whether the title describes the current
behavior or the correct behavior.

I think clearing m_linkPreloadResourceClient might get you the right behavior.
If m_linkPreloadResourceClient is the only ResourceClient associated with the
Resource, Resource::allClientsRemoved will get called, which should cancel the
ResourceLoader.

https://codereview.chromium.org/1615703002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/LinkLoader.cpp (right):

https://codereview.chromium.org/1615703002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/LinkLoader.cpp:325:
m_linkPreloadResourceClient->cancel();
Can you get the correct behavior here by just calling
m_linkPreloadResourceClient.clear()?

https://codereview.chromium.org/1615703002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/LinkPreloadResourceClients.h (right):

https://codereview.chromium.org/1615703002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/LinkPreloadResourceClients.h:70:
resource()->loader()->cancel();
This will cancel the load even if the preload is now being used by a real load.
That seems wrong?

[Yoav Weiss, 2016-01-21 19:58:58]

Description was changed from

==========
Removal of rel=preload link elements stops the download.

From the spec: https://w3c.github.io/preload/
"The user agent should abort the request if the href attribute on the link
element of an preload link is removed, or its value is set to an empty string."

This CL aligns the implementation with that part of the spec.

BUG=489646
==========

to

==========
Removal of rel=preload link elements should stop the download.

From the spec: https://w3c.github.io/preload/
"The user agent should abort the request if the href attribute on the link
element of an preload link is removed, or its value is set to an empty string."

This CL aligns the implementation with that part of the spec.

BUG=489646
==========

[Yoav Weiss, 2016-01-21 21:51:21]

Changed the subject and moved to clearResource() rather than cancel the loader.

Also added href changes handling, so that an empty or invalid href will also
trigger the same.

https://codereview.chromium.org/1615703002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/LinkLoader.cpp (right):

https://codereview.chromium.org/1615703002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/LinkLoader.cpp:325:
m_linkPreloadResourceClient->cancel();
On 2016/01/21 19:03:50, Nate Chapin wrote:
> Can you get the correct behavior here by just calling
> m_linkPreloadResourceClient.clear()?

Didn't really work, but clearResource inside the linkPreloadResourceClient did.

https://codereview.chromium.org/1615703002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/LinkPreloadResourceClients.h (right):

https://codereview.chromium.org/1615703002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/LinkPreloadResourceClients.h:70:
resource()->loader()->cancel();
On 2016/01/21 19:03:50, Nate Chapin wrote:
> This will cancel the load even if the preload is now being used by a real
load.
> That seems wrong?

yeah. changed to clearResource()

[Nate Chapin, 2016-01-21 21:53:17]

Are there downsides to deleting the LinkPreloadResourceClient instead?

https://codereview.chromium.org/1615703002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/LinkLoader.cpp (right):

https://codereview.chromium.org/1615703002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/LinkLoader.cpp:330:
m_linkPreloadResourceClient->clear();
I had actually meant deleting the LinkPreloadResourceClient, which should
implicitly call clearResource(). Sorry for not being more clear.

[Nate Chapin, 2016-01-21 21:53:41]

On 2016/01/21 21:53:17, Nate Chapin wrote:
> Are there downsides to deleting the LinkPreloadResourceClient instead?
> 
>
https://codereview.chromium.org/1615703002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/loader/LinkLoader.cpp (right):
> 
>
https://codereview.chromium.org/1615703002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/loader/LinkLoader.cpp:330:
> m_linkPreloadResourceClient->clear();
> I had actually meant deleting the LinkPreloadResourceClient, which should
> implicitly call clearResource(). Sorry for not being more clear.

....I misread your comments. Looking more carefully.

[Nate Chapin, 2016-01-21 21:55:44]

Ok, LGTM. I'm surprised deleting the LinkPreloadResourceClient doesn't work.
What went wrong exactly?

[Yoav Weiss, 2016-01-21 22:02:28]

On 2016/01/21 21:55:44, Nate Chapin wrote:
> Ok, LGTM. I'm surprised deleting the LinkPreloadResourceClient doesn't work.
> What went wrong exactly?

The test failed. I didn't thoroughly looked but it could be maybe
ResourceOwner's destructors are not properly running. I'll look further.

[Nate Chapin, 2016-01-21 22:09:57]

On 2016/01/21 22:02:28, Yoav Weiss wrote:
> On 2016/01/21 21:55:44, Nate Chapin wrote:
> > Ok, LGTM. I'm surprised deleting the LinkPreloadResourceClient doesn't work.
> > What went wrong exactly?
> 
> The test failed. I didn't thoroughly looked but it could be maybe
> ResourceOwner's destructors are not properly running. I'll look further.

The LinkPreloadResourceClients don't explicitly define a destructor. I can't
remember my C++ arcana, will the default destructor be empty and non-virtual by
any chance?

[Yoav Weiss, 2016-01-21 22:17:21]

On 2016/01/21 22:09:57, Nate Chapin wrote:
> On 2016/01/21 22:02:28, Yoav Weiss wrote:
> > On 2016/01/21 21:55:44, Nate Chapin wrote:
> > > Ok, LGTM. I'm surprised deleting the LinkPreloadResourceClient doesn't
work.
> > > What went wrong exactly?
> > 
> > The test failed. I didn't thoroughly looked but it could be maybe
> > ResourceOwner's destructors are not properly running. I'll look further.
> 
> The LinkPreloadResourceClients don't explicitly define a destructor. I can't
> remember my C++ arcana, will the default destructor be empty and non-virtual
by
> any chance?

That is probably it! (and could very well explain the mem leaks we saw in a
parallel CL)

[Yoav Weiss, 2016-01-22 01:48:38]

On 2016/01/21 22:17:21, Yoav Weiss wrote:
> On 2016/01/21 22:09:57, Nate Chapin wrote:
> > On 2016/01/21 22:02:28, Yoav Weiss wrote:
> > > On 2016/01/21 21:55:44, Nate Chapin wrote:
> > > > Ok, LGTM. I'm surprised deleting the LinkPreloadResourceClient doesn't
> work.
> > > > What went wrong exactly?
> > > 
> > > The test failed. I didn't thoroughly looked but it could be maybe
> > > ResourceOwner's destructors are not properly running. I'll look further.
> > 
> > The LinkPreloadResourceClients don't explicitly define a destructor. I can't
> > remember my C++ arcana, will the default destructor be empty and non-virtual
> by
> > any chance?
> 
> That is probably it! (and could very well explain the mem leaks we saw in a
> parallel CL)

Adding the virtual destructors did the trick for non-Oilpan, but with Oilpan, we
can't really rely on the destructor to run when we want it to.

I'll go back to calling clearResource explicitly.

[Yoav Weiss, 2016-01-22 03:01:51]

The CQ bit was checked by yoav@yoav.ws

[Yoav Weiss, 2016-01-22 03:01:52]

The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org
Link to the patchset: https://codereview.chromium.org/1615703002/#ps80001
(title: "Added back clear()")

[commit-bot: I haz the power, 2016-01-22 03:03:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1615703002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1615703002/80001

[commit-bot: I haz the power, 2016-01-22 05:08:05]

Description was changed from

==========
Removal of rel=preload link elements should stop the download.

From the spec: https://w3c.github.io/preload/
"The user agent should abort the request if the href attribute on the link
element of an preload link is removed, or its value is set to an empty string."

This CL aligns the implementation with that part of the spec.

BUG=489646
==========

to

==========
Removal of rel=preload link elements should stop the download.

From the spec: https://w3c.github.io/preload/
"The user agent should abort the request if the href attribute on the link
element of an preload link is removed, or its value is set to an empty string."

This CL aligns the implementation with that part of the spec.

BUG=489646
==========

[commit-bot: I haz the power, 2016-01-22 05:08:06]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-01-22 05:09:04]

Description was changed from

==========
Removal of rel=preload link elements should stop the download.

From the spec: https://w3c.github.io/preload/
"The user agent should abort the request if the href attribute on the link
element of an preload link is removed, or its value is set to an empty string."

This CL aligns the implementation with that part of the spec.

BUG=489646
==========

to

==========
Removal of rel=preload link elements should stop the download.

From the spec: https://w3c.github.io/preload/
"The user agent should abort the request if the href attribute on the link
element of an preload link is removed, or its value is set to an empty string."

This CL aligns the implementation with that part of the spec.

BUG=489646

Committed: https://crrev.com/4548d907a4d13c34bc290c77753c30cf9adaceda
Cr-Commit-Position: refs/heads/master@{#370909}
==========

[commit-bot: I haz the power, 2016-01-22 05:09:05]

Patchset 5 (id:??) landed as
https://crrev.com/4548d907a4d13c34bc290c77753c30cf9adaceda
Cr-Commit-Position: refs/heads/master@{#370909}