Fix crash in CSSStyleSheetResource::appendData

This patch removes the preloader resource client from a resources clients before it is completely destroyed.

BUG=608310

[Charlie Harrison, 2016-05-02 12:55:15]

Description was changed from

==========
Early return in CSSStyleSheetResource::appendData

BUG=608310
==========

to

==========
Early return in CSSStyleSheetResource::appendData with no m_data

This patch early returns before notifying clients that data was
appended, in cases where Resource::appendData does not result
in creating the resources m_data member.

BUG=608310
==========

[Charlie Harrison, 2016-05-02 12:55:15]

csharrison@chromium.org changed reviewers:
+ japhet@chromium.org

[Charlie Harrison, 2016-05-02 13:58:55]

japhet, PTAL. I'm not sure if this is the cause of the crashes, but I believe
crashes will happen without this check.

[Charlie Harrison, 2016-05-02 13:59:33]

cc hiroshige@

[Nate Chapin, 2016-05-02 19:00:43]

If the problem is either of the theories mentioned in the comments below, it
should be possible to write a unit test to verify the root cause.

https://codereview.chromium.org/1937033002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.cpp (right):

https://codereview.chromium.org/1937033002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.cpp:105: if (!m_data)
This should only be possible when m_options.dataBufferingPolicy ==
DoNotBufferData, but it doesn't look like CSSStyleSheetResource loads ever use
DoNotBufferData.

https://codereview.chromium.org/1937033002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.cpp:111:
c->didAppendFirstData(this);
I'm wondering whether this bug is happening when there are multiple m_clients,
and one of the clients cancels the load (thus clearing m_data) inside
didAppendFirstData(), causing the next client to crash.

[Charlie Harrison, 2016-05-02 19:27:56]

csharrison@chromium.org changed reviewers:
+ kouhei@chromium.org

[Charlie Harrison, 2016-05-02 19:27:56]

Thanks for the comments. Now I'm not sure either of these options can happen.

So, the CSSPreloaderResourceClients are the only ones that get the
didAppendFirstData call.

They are scoped the HTMLResourcePreloader (which is scoped to the
HTMLDocumentParser). I think possibly what is happening is that the Resources
are outliving the HTMLResourcePreloader, and this is a use-after-free.

I'm not quite sure when the parser is detached from the document and resources
are still needed. I'd love to check this with a layout test. Any pointers?

+kouhei@ for parser stuff.

https://codereview.chromium.org/1937033002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.cpp (right):

https://codereview.chromium.org/1937033002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.cpp:105: if (!m_data)
On 2016/05/02 19:00:43, Nate Chapin wrote:
> This should only be possible when m_options.dataBufferingPolicy ==
> DoNotBufferData, but it doesn't look like CSSStyleSheetResource loads ever use
> DoNotBufferData.

Ah good point.

https://codereview.chromium.org/1937033002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fetch/CSSStyleSheetResource.cpp:111:
c->didAppendFirstData(this);
On 2016/05/02 19:00:43, Nate Chapin wrote:
> I'm wondering whether this bug is happening when there are multiple m_clients,
> and one of the clients cancels the load (thus clearing m_data) inside
> didAppendFirstData(), causing the next client to crash.

Hm well there's only one client of didAppendFirstData and they don't cancel any
loads.

[Charlie Harrison, 2016-05-02 19:27:58]

Thanks for the comments. Now I'm not sure either of these options can happen.

So, the CSSPreloaderResourceClients are the only ones that get the
didAppendFirstData call.

They are scoped the HTMLResourcePreloader (which is scoped to the
HTMLDocumentParser). I think possibly what is happening is that the Resources
are outliving the HTMLResourcePreloader, and this is a use-after-free.

I'm not quite sure when the parser is detached from the document and resources
are still needed. I'd love to check this with a layout test. Any pointers?

+kouhei@ for parser stuff.

[Charlie Harrison, 2016-05-02 23:02:01]

Description was changed from

==========
Early return in CSSStyleSheetResource::appendData with no m_data

This patch early returns before notifying clients that data was
appended, in cases where Resource::appendData does not result
in creating the resources m_data member.

BUG=608310
==========

to

==========
This patch removes the preloader resource client from a resources clients before
it is completely destroyed.

BUG=608310
==========

[Charlie Harrison, 2016-05-03 18:22:23]

As described in the bug, I've confirmed that this solution fixes the repro I
triggered (with a new navigation).

[Nate Chapin, 2016-05-03 18:25:30]

https://codereview.chromium.org/1937033002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/parser/CSSPreloadScanner.cpp (right):

https://codereview.chromium.org/1937033002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/parser/CSSPreloadScanner.cpp:255:
stopObserving();
Alternately, you can have CSSPreloaderResourceClient inherit
ResourceOwner<CSSStyleSheetResource>.

[Charlie Harrison, 2016-05-03 20:32:11]

I've converted the client to be a resource owner. I can only repro this with a
window.location.reload() spinning loop. Is that type of layout test reasonable?
It can be deterministic by e.g. limiting to 10 reloads.

https://codereview.chromium.org/1937033002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/parser/CSSPreloadScanner.cpp (right):

https://codereview.chromium.org/1937033002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/parser/CSSPreloadScanner.cpp:255:
stopObserving();
On 2016/05/03 18:25:30, Nate Chapin wrote:
> Alternately, you can have CSSPreloaderResourceClient inherit
> ResourceOwner<CSSStyleSheetResource>.

Very good point. Thanks for the suggestion.

[Nate Chapin, 2016-05-03 20:52:02]

On 2016/05/03 20:32:11, csharrison wrote:
> I've converted the client to be a resource owner. I can only repro this with a
> window.location.reload() spinning loop. Is that type of layout test
reasonable?
> It can be deterministic by e.g. limiting to 10 reloads.
> 

That's suboptimal. Maybe it's possible to write a unit test that
creates/destroys a CSSPreloadResourceClient directly and makes sure it doesn't
UAF?

>
https://codereview.chromium.org/1937033002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/html/parser/CSSPreloadScanner.cpp (right):
> 
>
https://codereview.chromium.org/1937033002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/html/parser/CSSPreloadScanner.cpp:255:
> stopObserving();
> On 2016/05/03 18:25:30, Nate Chapin wrote:
> > Alternately, you can have CSSPreloaderResourceClient inherit
> > ResourceOwner<CSSStyleSheetResource>.
> 
> Very good point. Thanks for the suggestion.

[Nate Chapin, 2016-05-03 20:52:13]

https://codereview.chromium.org/1937033002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/parser/HTMLResourcePreloader.h (right):

https://codereview.chromium.org/1937033002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/parser/HTMLResourcePreloader.h:54:
HeapVector<Member<CSSPreloaderResourceClient>> m_cssPreloaders;
Why not HeapHashSet?

[Charlie Harrison, 2016-05-04 13:26:36]

Going to close this, as I'm reverting the original patch. I'll merge this change
in with the original in a re-land and try to get some unit tests up.

https://codereview.chromium.org/1937033002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/parser/HTMLResourcePreloader.h (right):

https://codereview.chromium.org/1937033002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/parser/HTMLResourcePreloader.h:54:
HeapVector<Member<CSSPreloaderResourceClient>> m_cssPreloaders;
On 2016/05/03 20:52:13, Nate Chapin wrote:
> Why not HeapHashSet?

I don't think it's necessary, so I just went with the simplest expandable data
structure available.