Better handle reentrancy into DocumentLoader::dataReceived().

Better handle reentrancy into DocumentLoader::dataReceived().

dataReceived() can be called reentrantly if it triggers a nested message
loop when detaching a Document or parsing script for the new Document.
Since dataReceived() isn't safe for reentrancy, any nested invocations
queue the received data to be processed by the top-level invocation of
dataReceived().

BUG=515876
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=200436

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=200603

[dcheng, 2015-08-04 21:32:40]

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

[dcheng, 2015-08-04 21:38:29]

dcheng@chromium.org changed reviewers:
+ kouhei@chromium.org, morrita@chromium.org

[dcheng, 2015-08-05 00:18:13]

+japhet for primary review.

+kouhei, +morrita: Even with the threaded HTML parser, I assume that if we hit a
<script> block, that would still be executed on the main thread, right? So that
means even if we've already committed the load, appending data to the parser in
DocumentLoader::commitData() would execute script, possibly creating a nested
message loop.

Also, I don't have a test yet. I'm not sure what a good way to write this test
would be. Is there an example of how to mock out resource loads?

[kouhei (in TOK), 2015-08-05 05:22:11]

> +kouhei, +morrita: Even with the threaded HTML parser, I assume that if we hit
a
> <script> block, that would still be executed on the main thread, right?
Correct.

> So that
> means even if we've already committed the load, appending data to the parser
in
> DocumentLoader::commitData() would execute script, possibly creating a nested
> message loop.
Yes.

[Nate Chapin, 2015-08-05 18:27:07]

https://codereview.chromium.org/1263363005/diff/40001/Source/core/loader/Docu...
File Source/core/loader/DocumentLoader.h (right):

https://codereview.chromium.org/1263363005/diff/40001/Source/core/loader/Docu...
Source/core/loader/DocumentLoader.h:242: Deque<OwnPtr<Vector<char>>>
m_dataQueue;
I assume the OwnPtr here is actually necessary (or at least saves a copy)?

Can/should we use SharedBuffer here for readability?

[dcheng, 2015-08-05 21:57:12]

https://codereview.chromium.org/1263363005/diff/40001/Source/core/loader/Docu...
File Source/core/loader/DocumentLoader.h (right):

https://codereview.chromium.org/1263363005/diff/40001/Source/core/loader/Docu...
Source/core/loader/DocumentLoader.h:242: Deque<OwnPtr<Vector<char>>>
m_dataQueue;
On 2015/08/05 at 18:27:06, Nate Chapin wrote:
> I assume the OwnPtr here is actually necessary (or at least saves a copy)?
> 
> Can/should we use SharedBuffer here for readability?

It's to save a copy. If we could use std::move() and Deque::append() had a
move-only equivalent like std::vector::emplace_back, we wouldn't need this.

One alternative is to do:
  m_dataQueue.append(Vector());
  m_dataQueue.last().swap(buffer);
which would also avoid the copy, and allow us to simplify the type to
Deque<Vector<char>>.

Using Dequeue<RefPtr<SharedBuffer>> doesn't really seem better: the refcounting
is unnecessary.

As a final option, if it's not important to preserve the exact chunking, I can
just dump everything into a Deque<char>. This means multiple calls to
dataReceived() might get batched, but maybe it doesn't matter?

[Nate Chapin, 2015-08-05 21:59:37]

On 2015/08/05 21:57:12, dcheng wrote:
>
https://codereview.chromium.org/1263363005/diff/40001/Source/core/loader/Docu...
> File Source/core/loader/DocumentLoader.h (right):
> 
>
https://codereview.chromium.org/1263363005/diff/40001/Source/core/loader/Docu...
> Source/core/loader/DocumentLoader.h:242: Deque<OwnPtr<Vector<char>>>
> m_dataQueue;
> On 2015/08/05 at 18:27:06, Nate Chapin wrote:
> > I assume the OwnPtr here is actually necessary (or at least saves a copy)?
> > 
> > Can/should we use SharedBuffer here for readability?
> 
> It's to save a copy. If we could use std::move() and Deque::append() had a
> move-only equivalent like std::vector::emplace_back, we wouldn't need this.
> 
> One alternative is to do:
>   m_dataQueue.append(Vector());
>   m_dataQueue.last().swap(buffer);
> which would also avoid the copy, and allow us to simplify the type to
> Deque<Vector<char>>.
> 
> Using Dequeue<RefPtr<SharedBuffer>> doesn't really seem better: the
refcounting
> is unnecessary.
> 
> As a final option, if it's not important to preserve the exact chunking, I can
> just dump everything into a Deque<char>. This means multiple calls to
> dataReceived() might get batched, but maybe it doesn't matter?

It shouldn't matter. That doesn't mean it doesn't matter. :)

[Nate Chapin, 2015-08-05 21:59:56]

On 2015/08/05 21:59:37, Nate Chapin wrote:
> On 2015/08/05 21:57:12, dcheng wrote:
> >
>
https://codereview.chromium.org/1263363005/diff/40001/Source/core/loader/Docu...
> > File Source/core/loader/DocumentLoader.h (right):
> > 
> >
>
https://codereview.chromium.org/1263363005/diff/40001/Source/core/loader/Docu...
> > Source/core/loader/DocumentLoader.h:242: Deque<OwnPtr<Vector<char>>>
> > m_dataQueue;
> > On 2015/08/05 at 18:27:06, Nate Chapin wrote:
> > > I assume the OwnPtr here is actually necessary (or at least saves a copy)?
> > > 
> > > Can/should we use SharedBuffer here for readability?
> > 
> > It's to save a copy. If we could use std::move() and Deque::append() had a
> > move-only equivalent like std::vector::emplace_back, we wouldn't need this.
> > 
> > One alternative is to do:
> >   m_dataQueue.append(Vector());
> >   m_dataQueue.last().swap(buffer);
> > which would also avoid the copy, and allow us to simplify the type to
> > Deque<Vector<char>>.
> > 
> > Using Dequeue<RefPtr<SharedBuffer>> doesn't really seem better: the
> refcounting
> > is unnecessary.
> > 
> > As a final option, if it's not important to preserve the exact chunking, I
can
> > just dump everything into a Deque<char>. This means multiple calls to
> > dataReceived() might get batched, but maybe it doesn't matter?
> 
> It shouldn't matter. That doesn't mean it doesn't matter. :)

...which is to say, let's try it.

[dcheng, 2015-08-05 22:19:20]

On 2015/08/05 at 21:59:56, japhet wrote:
> On 2015/08/05 21:59:37, Nate Chapin wrote:
> > On 2015/08/05 21:57:12, dcheng wrote:
> > >
> >
https://codereview.chromium.org/1263363005/diff/40001/Source/core/loader/Docu...
> > > File Source/core/loader/DocumentLoader.h (right):
> > > 
> > >
> >
https://codereview.chromium.org/1263363005/diff/40001/Source/core/loader/Docu...
> > > Source/core/loader/DocumentLoader.h:242: Deque<OwnPtr<Vector<char>>>
> > > m_dataQueue;
> > > On 2015/08/05 at 18:27:06, Nate Chapin wrote:
> > > > I assume the OwnPtr here is actually necessary (or at least saves a
copy)?
> > > > 
> > > > Can/should we use SharedBuffer here for readability?
> > > 
> > > It's to save a copy. If we could use std::move() and Deque::append() had a
> > > move-only equivalent like std::vector::emplace_back, we wouldn't need
this.
> > > 
> > > One alternative is to do:
> > >   m_dataQueue.append(Vector());
> > >   m_dataQueue.last().swap(buffer);
> > > which would also avoid the copy, and allow us to simplify the type to
> > > Deque<Vector<char>>.
> > > 
> > > Using Dequeue<RefPtr<SharedBuffer>> doesn't really seem better: the
> > refcounting
> > > is unnecessary.
> > > 
> > > As a final option, if it's not important to preserve the exact chunking, I
can
> > > just dump everything into a Deque<char>. This means multiple calls to
> > > dataReceived() might get batched, but maybe it doesn't matter?
> > 
> > It shouldn't matter. That doesn't mean it doesn't matter. :)
> 
> ...which is to say, let's try it.

So I have bad news...

Unlike std::deque, WTF::Deque doesn't provide a way to bulk remove elements. So
I'd have to call removeFirst() n times to consume n bytes from it.

SharedBuffer doesn't provide a way to "consume" bytes outside of calling
clear(). So in worst-case scenarios, it could pin a significant chunk of stuff
into memory for quite some time.

StreamBuffer seems like it might fit the bill... except it's currently
completely unused (there's not even a test). The template definition also
requires specifying a block size that gets used for each chunk.

[Nate Chapin, 2015-08-05 22:23:08]

On 2015/08/05 22:19:20, dcheng wrote:
> On 2015/08/05 at 21:59:56, japhet wrote:
> > On 2015/08/05 21:59:37, Nate Chapin wrote:
> > > On 2015/08/05 21:57:12, dcheng wrote:
> > > >
> > >
>
https://codereview.chromium.org/1263363005/diff/40001/Source/core/loader/Docu...
> > > > File Source/core/loader/DocumentLoader.h (right):
> > > > 
> > > >
> > >
>
https://codereview.chromium.org/1263363005/diff/40001/Source/core/loader/Docu...
> > > > Source/core/loader/DocumentLoader.h:242: Deque<OwnPtr<Vector<char>>>
> > > > m_dataQueue;
> > > > On 2015/08/05 at 18:27:06, Nate Chapin wrote:
> > > > > I assume the OwnPtr here is actually necessary (or at least saves a
> copy)?
> > > > > 
> > > > > Can/should we use SharedBuffer here for readability?
> > > > 
> > > > It's to save a copy. If we could use std::move() and Deque::append() had
a
> > > > move-only equivalent like std::vector::emplace_back, we wouldn't need
> this.
> > > > 
> > > > One alternative is to do:
> > > >   m_dataQueue.append(Vector());
> > > >   m_dataQueue.last().swap(buffer);
> > > > which would also avoid the copy, and allow us to simplify the type to
> > > > Deque<Vector<char>>.
> > > > 
> > > > Using Dequeue<RefPtr<SharedBuffer>> doesn't really seem better: the
> > > refcounting
> > > > is unnecessary.
> > > > 
> > > > As a final option, if it's not important to preserve the exact chunking,
I
> can
> > > > just dump everything into a Deque<char>. This means multiple calls to
> > > > dataReceived() might get batched, but maybe it doesn't matter?
> > > 
> > > It shouldn't matter. That doesn't mean it doesn't matter. :)
> > 
> > ...which is to say, let's try it.
> 
> So I have bad news...
> 
> Unlike std::deque, WTF::Deque doesn't provide a way to bulk remove elements.
So
> I'd have to call removeFirst() n times to consume n bytes from it.
> 
> SharedBuffer doesn't provide a way to "consume" bytes outside of calling
> clear(). So in worst-case scenarios, it could pin a significant chunk of stuff
> into memory for quite some time.
> 
> StreamBuffer seems like it might fit the bill... except it's currently
> completely unused (there's not even a test). The template definition also
> requires specifying a block size that gets used for each chunk.

Is there ever a situation in which m_dataQueue should be non-empty when the
stack unwinds? If it's guaranteed to be fully consumed when dataReceived()
exits, I'd say the SharedBuffer shortcomings are unimportant.

[dcheng, 2015-08-05 22:40:20]

On 2015/08/05 at 22:23:08, japhet wrote:
> On 2015/08/05 22:19:20, dcheng wrote:
> > On 2015/08/05 at 21:59:56, japhet wrote:
> > > On 2015/08/05 21:59:37, Nate Chapin wrote:
> > > > On 2015/08/05 21:57:12, dcheng wrote:
> > > > >
> > > >
> >
https://codereview.chromium.org/1263363005/diff/40001/Source/core/loader/Docu...
> > > > > File Source/core/loader/DocumentLoader.h (right):
> > > > > 
> > > > >
> > > >
> >
https://codereview.chromium.org/1263363005/diff/40001/Source/core/loader/Docu...
> > > > > Source/core/loader/DocumentLoader.h:242: Deque<OwnPtr<Vector<char>>>
> > > > > m_dataQueue;
> > > > > On 2015/08/05 at 18:27:06, Nate Chapin wrote:
> > > > > > I assume the OwnPtr here is actually necessary (or at least saves a
> > copy)?
> > > > > > 
> > > > > > Can/should we use SharedBuffer here for readability?
> > > > > 
> > > > > It's to save a copy. If we could use std::move() and Deque::append()
had a
> > > > > move-only equivalent like std::vector::emplace_back, we wouldn't need
> > this.
> > > > > 
> > > > > One alternative is to do:
> > > > >   m_dataQueue.append(Vector());
> > > > >   m_dataQueue.last().swap(buffer);
> > > > > which would also avoid the copy, and allow us to simplify the type to
> > > > > Deque<Vector<char>>.
> > > > > 
> > > > > Using Dequeue<RefPtr<SharedBuffer>> doesn't really seem better: the
> > > > refcounting
> > > > > is unnecessary.
> > > > > 
> > > > > As a final option, if it's not important to preserve the exact
chunking, I
> > can
> > > > > just dump everything into a Deque<char>. This means multiple calls to
> > > > > dataReceived() might get batched, but maybe it doesn't matter?
> > > > 
> > > > It shouldn't matter. That doesn't mean it doesn't matter. :)
> > > 
> > > ...which is to say, let's try it.
> > 
> > So I have bad news...
> > 
> > Unlike std::deque, WTF::Deque doesn't provide a way to bulk remove elements.
So
> > I'd have to call removeFirst() n times to consume n bytes from it.
> > 
> > SharedBuffer doesn't provide a way to "consume" bytes outside of calling
> > clear(). So in worst-case scenarios, it could pin a significant chunk of
stuff
> > into memory for quite some time.
> > 
> > StreamBuffer seems like it might fit the bill... except it's currently
> > completely unused (there's not even a test). The template definition also
> > requires specifying a block size that gets used for each chunk.
> 
> Is there ever a situation in which m_dataQueue should be non-empty when the
stack unwinds? If it's guaranteed to be fully consumed when dataReceived()
exits, I'd say the SharedBuffer shortcomings are unimportant.

It should always be empty at the completion of the top-level invocation of
dataReceived(), so I've changed it to SharedBuffer.

I don't suppose we have a good way of mocking out resource loads though? It'd be
great to be able to test this: my first version forgot to clear() the buffer.

[Nate Chapin, 2015-08-05 22:42:15]

On 2015/08/05 22:40:20, dcheng wrote:
> On 2015/08/05 at 22:23:08, japhet wrote:
> > On 2015/08/05 22:19:20, dcheng wrote:
> > > On 2015/08/05 at 21:59:56, japhet wrote:
> > > > On 2015/08/05 21:59:37, Nate Chapin wrote:
> > > > > On 2015/08/05 21:57:12, dcheng wrote:
> > > > > >
> > > > >
> > >
>
https://codereview.chromium.org/1263363005/diff/40001/Source/core/loader/Docu...
> > > > > > File Source/core/loader/DocumentLoader.h (right):
> > > > > > 
> > > > > >
> > > > >
> > >
>
https://codereview.chromium.org/1263363005/diff/40001/Source/core/loader/Docu...
> > > > > > Source/core/loader/DocumentLoader.h:242: Deque<OwnPtr<Vector<char>>>
> > > > > > m_dataQueue;
> > > > > > On 2015/08/05 at 18:27:06, Nate Chapin wrote:
> > > > > > > I assume the OwnPtr here is actually necessary (or at least saves
a
> > > copy)?
> > > > > > > 
> > > > > > > Can/should we use SharedBuffer here for readability?
> > > > > > 
> > > > > > It's to save a copy. If we could use std::move() and Deque::append()
> had a
> > > > > > move-only equivalent like std::vector::emplace_back, we wouldn't
need
> > > this.
> > > > > > 
> > > > > > One alternative is to do:
> > > > > >   m_dataQueue.append(Vector());
> > > > > >   m_dataQueue.last().swap(buffer);
> > > > > > which would also avoid the copy, and allow us to simplify the type
to
> > > > > > Deque<Vector<char>>.
> > > > > > 
> > > > > > Using Dequeue<RefPtr<SharedBuffer>> doesn't really seem better: the
> > > > > refcounting
> > > > > > is unnecessary.
> > > > > > 
> > > > > > As a final option, if it's not important to preserve the exact
> chunking, I
> > > can
> > > > > > just dump everything into a Deque<char>. This means multiple calls
to
> > > > > > dataReceived() might get batched, but maybe it doesn't matter?
> > > > > 
> > > > > It shouldn't matter. That doesn't mean it doesn't matter. :)
> > > > 
> > > > ...which is to say, let's try it.
> > > 
> > > So I have bad news...
> > > 
> > > Unlike std::deque, WTF::Deque doesn't provide a way to bulk remove
elements.
> So
> > > I'd have to call removeFirst() n times to consume n bytes from it.
> > > 
> > > SharedBuffer doesn't provide a way to "consume" bytes outside of calling
> > > clear(). So in worst-case scenarios, it could pin a significant chunk of
> stuff
> > > into memory for quite some time.
> > > 
> > > StreamBuffer seems like it might fit the bill... except it's currently
> > > completely unused (there's not even a test). The template definition also
> > > requires specifying a block size that gets used for each chunk.
> > 
> > Is there ever a situation in which m_dataQueue should be non-empty when the
> stack unwinds? If it's guaranteed to be fully consumed when dataReceived()
> exits, I'd say the SharedBuffer shortcomings are unimportant.
> 
> It should always be empty at the completion of the top-level invocation of
> dataReceived(), so I've changed it to SharedBuffer.
> 
> I don't suppose we have a good way of mocking out resource loads though? It'd
be
> great to be able to test this: my first version forgot to clear() the buffer.

What part are you trying to mock out, the multiple chunks?

[dcheng, 2015-08-05 22:46:54]

On 2015/08/05 at 22:42:15, japhet wrote:
> On 2015/08/05 22:40:20, dcheng wrote:
> > On 2015/08/05 at 22:23:08, japhet wrote:
> > > On 2015/08/05 22:19:20, dcheng wrote:
> > > > On 2015/08/05 at 21:59:56, japhet wrote:
> > > > > On 2015/08/05 21:59:37, Nate Chapin wrote:
> > > > > > On 2015/08/05 21:57:12, dcheng wrote:
> > > > > > >
> > > > > >
> > > >
> >
https://codereview.chromium.org/1263363005/diff/40001/Source/core/loader/Docu...
> > > > > > > File Source/core/loader/DocumentLoader.h (right):
> > > > > > > 
> > > > > > >
> > > > > >
> > > >
> >
https://codereview.chromium.org/1263363005/diff/40001/Source/core/loader/Docu...
> > > > > > > Source/core/loader/DocumentLoader.h:242:
Deque<OwnPtr<Vector<char>>>
> > > > > > > m_dataQueue;
> > > > > > > On 2015/08/05 at 18:27:06, Nate Chapin wrote:
> > > > > > > > I assume the OwnPtr here is actually necessary (or at least
saves a
> > > > copy)?
> > > > > > > > 
> > > > > > > > Can/should we use SharedBuffer here for readability?
> > > > > > > 
> > > > > > > It's to save a copy. If we could use std::move() and
Deque::append()
> > had a
> > > > > > > move-only equivalent like std::vector::emplace_back, we wouldn't
need
> > > > this.
> > > > > > > 
> > > > > > > One alternative is to do:
> > > > > > >   m_dataQueue.append(Vector());
> > > > > > >   m_dataQueue.last().swap(buffer);
> > > > > > > which would also avoid the copy, and allow us to simplify the type
to
> > > > > > > Deque<Vector<char>>.
> > > > > > > 
> > > > > > > Using Dequeue<RefPtr<SharedBuffer>> doesn't really seem better:
the
> > > > > > refcounting
> > > > > > > is unnecessary.
> > > > > > > 
> > > > > > > As a final option, if it's not important to preserve the exact
> > chunking, I
> > > > can
> > > > > > > just dump everything into a Deque<char>. This means multiple calls
to
> > > > > > > dataReceived() might get batched, but maybe it doesn't matter?
> > > > > > 
> > > > > > It shouldn't matter. That doesn't mean it doesn't matter. :)
> > > > > 
> > > > > ...which is to say, let's try it.
> > > > 
> > > > So I have bad news...
> > > > 
> > > > Unlike std::deque, WTF::Deque doesn't provide a way to bulk remove
elements.
> > So
> > > > I'd have to call removeFirst() n times to consume n bytes from it.
> > > > 
> > > > SharedBuffer doesn't provide a way to "consume" bytes outside of calling
> > > > clear(). So in worst-case scenarios, it could pin a significant chunk of
> > stuff
> > > > into memory for quite some time.
> > > > 
> > > > StreamBuffer seems like it might fit the bill... except it's currently
> > > > completely unused (there's not even a test). The template definition
also
> > > > requires specifying a block size that gets used for each chunk.
> > > 
> > > Is there ever a situation in which m_dataQueue should be non-empty when
the
> > stack unwinds? If it's guaranteed to be fully consumed when dataReceived()
> > exits, I'd say the SharedBuffer shortcomings are unimportant.
> > 
> > It should always be empty at the completion of the top-level invocation of
> > dataReceived(), so I've changed it to SharedBuffer.
> > 
> > I don't suppose we have a good way of mocking out resource loads though?
It'd be
> > great to be able to test this: my first version forgot to clear() the
buffer.
> 
> What part are you trying to mock out, the multiple chunks?

Basically, I'd like to be able to do something like this:
Send first x bytes of a new document, which will detach the old Document.
Hook WebFrameClient::frameDetached() to call back into
DocumentLoader::dataReceived() and supply another y bytes.
Allow the stack to completely unwind and then send the final z bytes.

[Nate Chapin, 2015-08-05 22:52:05]

On 2015/08/05 22:46:54, dcheng wrote:
> On 2015/08/05 at 22:42:15, japhet wrote:
> > On 2015/08/05 22:40:20, dcheng wrote:
> > > On 2015/08/05 at 22:23:08, japhet wrote:
> > > > On 2015/08/05 22:19:20, dcheng wrote:
> > > > > On 2015/08/05 at 21:59:56, japhet wrote:
> > > > > > On 2015/08/05 21:59:37, Nate Chapin wrote:
> > > > > > > On 2015/08/05 21:57:12, dcheng wrote:
> > > > > > > >
> > > > > > >
> > > > >
> > >
>
https://codereview.chromium.org/1263363005/diff/40001/Source/core/loader/Docu...
> > > > > > > > File Source/core/loader/DocumentLoader.h (right):
> > > > > > > > 
> > > > > > > >
> > > > > > >
> > > > >
> > >
>
https://codereview.chromium.org/1263363005/diff/40001/Source/core/loader/Docu...
> > > > > > > > Source/core/loader/DocumentLoader.h:242:
> Deque<OwnPtr<Vector<char>>>
> > > > > > > > m_dataQueue;
> > > > > > > > On 2015/08/05 at 18:27:06, Nate Chapin wrote:
> > > > > > > > > I assume the OwnPtr here is actually necessary (or at least
> saves a
> > > > > copy)?
> > > > > > > > > 
> > > > > > > > > Can/should we use SharedBuffer here for readability?
> > > > > > > > 
> > > > > > > > It's to save a copy. If we could use std::move() and
> Deque::append()
> > > had a
> > > > > > > > move-only equivalent like std::vector::emplace_back, we wouldn't
> need
> > > > > this.
> > > > > > > > 
> > > > > > > > One alternative is to do:
> > > > > > > >   m_dataQueue.append(Vector());
> > > > > > > >   m_dataQueue.last().swap(buffer);
> > > > > > > > which would also avoid the copy, and allow us to simplify the
type
> to
> > > > > > > > Deque<Vector<char>>.
> > > > > > > > 
> > > > > > > > Using Dequeue<RefPtr<SharedBuffer>> doesn't really seem better:
> the
> > > > > > > refcounting
> > > > > > > > is unnecessary.
> > > > > > > > 
> > > > > > > > As a final option, if it's not important to preserve the exact
> > > chunking, I
> > > > > can
> > > > > > > > just dump everything into a Deque<char>. This means multiple
calls
> to
> > > > > > > > dataReceived() might get batched, but maybe it doesn't matter?
> > > > > > > 
> > > > > > > It shouldn't matter. That doesn't mean it doesn't matter. :)
> > > > > > 
> > > > > > ...which is to say, let's try it.
> > > > > 
> > > > > So I have bad news...
> > > > > 
> > > > > Unlike std::deque, WTF::Deque doesn't provide a way to bulk remove
> elements.
> > > So
> > > > > I'd have to call removeFirst() n times to consume n bytes from it.
> > > > > 
> > > > > SharedBuffer doesn't provide a way to "consume" bytes outside of
calling
> > > > > clear(). So in worst-case scenarios, it could pin a significant chunk
of
> > > stuff
> > > > > into memory for quite some time.
> > > > > 
> > > > > StreamBuffer seems like it might fit the bill... except it's currently
> > > > > completely unused (there's not even a test). The template definition
> also
> > > > > requires specifying a block size that gets used for each chunk.
> > > > 
> > > > Is there ever a situation in which m_dataQueue should be non-empty when
> the
> > > stack unwinds? If it's guaranteed to be fully consumed when dataReceived()
> > > exits, I'd say the SharedBuffer shortcomings are unimportant.
> > > 
> > > It should always be empty at the completion of the top-level invocation of
> > > dataReceived(), so I've changed it to SharedBuffer.
> > > 
> > > I don't suppose we have a good way of mocking out resource loads though?
> It'd be
> > > great to be able to test this: my first version forgot to clear() the
> buffer.
> > 
> > What part are you trying to mock out, the multiple chunks?
> 
> Basically, I'd like to be able to do something like this:
> Send first x bytes of a new document, which will detach the old Document.
> Hook WebFrameClient::frameDetached() to call back into
> DocumentLoader::dataReceived() and supply another y bytes.
> Allow the stack to completely unwind and then send the final z bytes.

Create large test file such that it will chunk (does the WebURLLoaderMock do
that?), load it in unit test, shove data into DocumentLoader via manual call to
dataReceived in WebFrameClient::frameDetached? Nasty layering violation, but
might work...

[dcheng, 2015-08-10 23:57:58]

PTAL, I added some tests using the WebURLLoaderTestDelegate glue I added.

[Nate Chapin, 2015-08-11 17:36:36]

https://codereview.chromium.org/1263363005/diff/160001/Source/web/tests/Docum...
File Source/web/tests/DocumentLoaderTest.cpp (right):

https://codereview.chromium.org/1263363005/diff/160001/Source/web/tests/Docum...
Source/web/tests/DocumentLoaderTest.cpp:20: class DocumentLoaderTest : public
::testing::Test {
It feels wrong to put this in web/ given the name.

https://codereview.chromium.org/1263363005/diff/160001/Source/web/tests/Docum...
Source/web/tests/DocumentLoaderTest.cpp:51: } delegate;
What does this syntax do? I'm not familiar with it.

[dcheng, 2015-08-11 17:41:37]

https://codereview.chromium.org/1263363005/diff/160001/Source/web/tests/Docum...
File Source/web/tests/DocumentLoaderTest.cpp (right):

https://codereview.chromium.org/1263363005/diff/160001/Source/web/tests/Docum...
Source/web/tests/DocumentLoaderTest.cpp:20: class DocumentLoaderTest : public
::testing::Test {
On 2015/08/11 at 17:36:36, Nate Chapin wrote:
> It feels wrong to put this in web/ given the name.

It does. The problem is:
- I can't put this test in core, since I need to use the machine in web/ for
loading.
- Naming the test something else seems weird, since this is trying to exercise
the new logic in DocumentLoader.

I'm open to any suggestions to make this situation less bad.

https://codereview.chromium.org/1263363005/diff/160001/Source/web/tests/Docum...
Source/web/tests/DocumentLoaderTest.cpp:51: } delegate;
On 2015/08/11 at 17:36:36, Nate Chapin wrote:
> What does this syntax do? I'm not familiar with it.

I just collapsed the definition of the class and the test delegate variable into
one statement.
If you think it'd be clearer, I can just put 'TestDelegate delegate'; on its own
line to avoid combining them like this.

[Nate Chapin, 2015-08-12 21:10:53]

LGTM

https://codereview.chromium.org/1263363005/diff/160001/Source/web/tests/Docum...
File Source/web/tests/DocumentLoaderTest.cpp (right):

https://codereview.chromium.org/1263363005/diff/160001/Source/web/tests/Docum...
Source/web/tests/DocumentLoaderTest.cpp:20: class DocumentLoaderTest : public
::testing::Test {
On 2015/08/11 17:41:37, dcheng wrote:
> On 2015/08/11 at 17:36:36, Nate Chapin wrote:
> > It feels wrong to put this in web/ given the name.
> 
> It does. The problem is:
> - I can't put this test in core, since I need to use the machine in web/ for
> loading.
> - Naming the test something else seems weird, since this is trying to exercise
> the new logic in DocumentLoader.
> 
> I'm open to any suggestions to make this situation less bad.

Ideally, we'd have enough of FrameTestHelpers in core that you could do this
without web. That's probably overkill for this patch.

A TODO/FIXME noting that this is a bad location should suffice.

[dcheng, 2015-08-12 22:06:41]

https://codereview.chromium.org/1263363005/diff/160001/Source/web/tests/Docum...
File Source/web/tests/DocumentLoaderTest.cpp (right):

https://codereview.chromium.org/1263363005/diff/160001/Source/web/tests/Docum...
Source/web/tests/DocumentLoaderTest.cpp:20: class DocumentLoaderTest : public
::testing::Test {
On 2015/08/12 at 21:10:53, Nate Chapin wrote:
> On 2015/08/11 17:41:37, dcheng wrote:
> > On 2015/08/11 at 17:36:36, Nate Chapin wrote:
> > > It feels wrong to put this in web/ given the name.
> > 
> > It does. The problem is:
> > - I can't put this test in core, since I need to use the machine in web/ for
> > loading.
> > - Naming the test something else seems weird, since this is trying to
exercise
> > the new logic in DocumentLoader.
> > 
> > I'm open to any suggestions to make this situation less bad.
> 
> Ideally, we'd have enough of FrameTestHelpers in core that you could do this
without web. That's probably overkill for this patch.
> 
> A TODO/FIXME noting that this is a bad location should suffice.

Done.

[dcheng, 2015-08-12 22:07:12]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2015-08-12 22:07:12]

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/1263363005/#ps180001
(title: ".")

[commit-bot: I haz the power, 2015-08-12 22:07:40]

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

[commit-bot: I haz the power, 2015-08-12 23:29:16]

Committed patchset #10 (id:180001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=200436

[sof, 2015-08-13 20:03:53]

A revert of this CL (patchset #10 id:180001) has been created in
https://codereview.chromium.org/1295553002/ by sigbjornf@opera.com.

The reason for reverting is: ASan bots are reporting a handful of UAFs,


http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20ASAN/bu....

[dcheng, 2015-08-14 17:12:44]

PTAL. There was a UaF bug in the original patch, since TemporaryChange was
destroyed after the stack protectors for DocumentLoader were destroyed.

[Nate Chapin, 2015-08-14 20:34:35]

On 2015/08/14 17:12:44, dcheng wrote:
> PTAL. There was a UaF bug in the original patch, since TemporaryChange was
> destroyed after the stack protectors for DocumentLoader were destroyed.

Ouch, yeah. I'm not surprised no one caught that the first time around. LGTM

[dcheng, 2015-08-15 05:05:40]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2015-08-15 05:05:56]

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

[commit-bot: I haz the power, 2015-08-15 06:51:52]

Committed patchset #12 (id:220001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=200603