Oilpan: move core/fileapi to oilpan's heap.

Oilpan: move core/fileapi to oilpan's heap.

This moves the File API and its objects to Oilpan using transition
types.

R=haraken@chromium.org,tkent@chromium.org
BUG=340522
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=168460

[sof, 2014-02-24 13:11:04]

When you have a spare moment next, please take a look.

This CL needs https://codereview.chromium.org/174963002/ to avoid running into a
leak, I'll rebase once it has landed.

[sof, 2014-02-24 13:12:42]

https://codereview.chromium.org/176853004/diff/1/Source/core/html/forms/FileI...
File Source/core/html/forms/FileInputType.h (right):

https://codereview.chromium.org/176853004/diff/1/Source/core/html/forms/FileI...
Source/core/html/forms/FileInputType.h:80: RefPtrWillBePersistent<FileList>
m_fileList;
This persistent isn't promptly released & GCed on shutdown. Unless there are
mechanisms to make that happen, this CL may have to wait until Node-based
objects have been moved to Oilpan.

[haraken, 2014-02-24 13:49:13]

https://codereview.chromium.org/176853004/diff/1/Source/core/fileapi/FileError.h
File Source/core/fileapi/FileError.h (right):

https://codereview.chromium.org/176853004/diff/1/Source/core/fileapi/FileErro...
Source/core/fileapi/FileError.h:44: class FileError : public DOMError {

Let's override a trace() method in all subclasses of a garbage-collected class
(even if the class does not have any Member).

https://codereview.chromium.org/176853004/diff/1/Source/core/fileapi/FileErro...
File Source/core/fileapi/FileError.idl (right):

https://codereview.chromium.org/176853004/diff/1/Source/core/fileapi/FileErro...
Source/core/fileapi/FileError.idl:32: WillBeGarbageCollected

You don't need to add [WillBeGarbageCollected] to subclasses.

https://codereview.chromium.org/176853004/diff/1/Source/core/fileapi/Stream.h
File Source/core/fileapi/Stream.h (right):

https://codereview.chromium.org/176853004/diff/1/Source/core/fileapi/Stream.h...
Source/core/fileapi/Stream.h:46: class Stream FINAL : public
RefCountedWillBeRefCountedGarbageCollected<Stream>, public ScriptWrappable,
public ActiveDOMObject {

Not related to this CL, but we should kill
ActiveDOMObject::{set,unset}PendingActivity at some point. We should replace
them with hasPendingActivity. {set,unset}PendingActivity is manually calling
ref/deref, which is easy to cause memory leaks. If we could kill
{set,unset}PendingActivity, we don't need to use RefCountedGarbageCollected for
ActiveDOMObjects.

https://codereview.chromium.org/176853004/diff/1/Source/core/html/FormDataList.h
File Source/core/html/FormDataList.h (right):

https://codereview.chromium.org/176853004/diff/1/Source/core/html/FormDataLis...
Source/core/html/FormDataList.h:35: class Item {

Shall we add ALLOW_ONLY_INLINE_ALLOCATION?

https://codereview.chromium.org/176853004/diff/1/Source/core/html/forms/FileI...
File Source/core/html/forms/FileInputType.h (right):

https://codereview.chromium.org/176853004/diff/1/Source/core/html/forms/FileI...
Source/core/html/forms/FileInputType.h:80: RefPtrWillBePersistent<FileList>
m_fileList;
On 2014/02/24 13:12:43, sof wrote:
> This persistent isn't promptly released & GCed on shutdown. Unless there are
> mechanisms to make that happen, this CL may have to wait until Node-based
> objects have been moved to Oilpan.

Why isn't FileInputType destructed on shutdown? Who retains the FileInputType?

https://codereview.chromium.org/176853004/diff/1/Source/core/xml/XMLHttpReque...
File Source/core/xml/XMLHttpRequest.h (right):

https://codereview.chromium.org/176853004/diff/1/Source/core/xml/XMLHttpReque...
Source/core/xml/XMLHttpRequest.h:227: RefPtrWillBePersistent<Stream>
m_responseStream;

XMLHttpRequest is already on the heap. Thus these should be Members.

https://codereview.chromium.org/176853004/diff/1/Source/modules/filesystem/DO...
File Source/modules/filesystem/DOMFileSystemSync.cpp (right):

https://codereview.chromium.org/176853004/diff/1/Source/modules/filesystem/DO...
Source/modules/filesystem/DOMFileSystemSync.cpp:175: return
result->m_file.get();

Shall we add an implicit conversion from a persistent handle to a raw pointer?

https://codereview.chromium.org/176853004/diff/1/Source/modules/filesystem/Fi...
File Source/modules/filesystem/FileWriter.h (right):

https://codereview.chromium.org/176853004/diff/1/Source/modules/filesystem/Fi...
Source/modules/filesystem/FileWriter.h:51: class FileWriter FINAL : public
ScriptWrappable, public FileWriterBase, public ActiveDOMObject, public
EventTargetWithInlineData, public blink::WebFileWriterClient {

I believe you're planning to move FileWriter to the heap in a follow-up :)

https://codereview.chromium.org/176853004/diff/1/public/web/WebBlob.h
File public/web/WebBlob.h (right):

https://codereview.chromium.org/176853004/diff/1/public/web/WebBlob.h#newcode79
public/web/WebBlob.h:79: operator PassRefPtrWillBeRawPtr<WebCore::Blob>() const;

Shall we remove the implicit conversion and instead implement getValue()?

https://codereview.chromium.org/176853004/diff/1/public/web/WebDOMError.h
File public/web/WebDOMError.h (right):

https://codereview.chromium.org/176853004/diff/1/public/web/WebDOMError.h#new...
public/web/WebDOMError.h:72: operator
PassRefPtrWillBeRawPtr<WebCore::DOMError>() const;

Ditto.

[sof, 2014-02-24 17:09:08]

https://codereview.chromium.org/176853004/diff/1/Source/core/fileapi/FileError.h
File Source/core/fileapi/FileError.h (right):

https://codereview.chromium.org/176853004/diff/1/Source/core/fileapi/FileErro...
Source/core/fileapi/FileError.h:44: class FileError : public DOMError {
On 2014/02/24 13:49:14, haraken wrote:
> 
> Let's override a trace() method in all subclasses of a garbage-collected class
> (even if the class does not have any Member).

Even if the class, like here, doesn't have any other virtual methods?

https://codereview.chromium.org/176853004/diff/1/Source/core/fileapi/FileErro...
File Source/core/fileapi/FileError.idl (right):

https://codereview.chromium.org/176853004/diff/1/Source/core/fileapi/FileErro...
Source/core/fileapi/FileError.idl:32: WillBeGarbageCollected
On 2014/02/24 13:49:14, haraken wrote:
> 
> You don't need to add [WillBeGarbageCollected] to subclasses.

Thanks, didn't know. Removed (same for File.)

https://codereview.chromium.org/176853004/diff/1/Source/core/fileapi/Stream.h
File Source/core/fileapi/Stream.h (right):

https://codereview.chromium.org/176853004/diff/1/Source/core/fileapi/Stream.h...
Source/core/fileapi/Stream.h:46: class Stream FINAL : public
RefCountedWillBeRefCountedGarbageCollected<Stream>, public ScriptWrappable,
public ActiveDOMObject {
On 2014/02/24 13:49:14, haraken wrote:
> 
> Not related to this CL, but we should kill
> ActiveDOMObject::{set,unset}PendingActivity at some point. We should replace
> them with hasPendingActivity. {set,unset}PendingActivity is manually calling
> ref/deref, which is easy to cause memory leaks. If we could kill
> {set,unset}PendingActivity, we don't need to use RefCountedGarbageCollected
for
> ActiveDOMObjects.

Not doing that bookkeeping correctly was the cause of
https://codereview.chromium.org/174963002/

https://codereview.chromium.org/176853004/diff/1/Source/core/html/FormDataList.h
File Source/core/html/FormDataList.h (right):

https://codereview.chromium.org/176853004/diff/1/Source/core/html/FormDataLis...
Source/core/html/FormDataList.h:35: class Item {
On 2014/02/24 13:49:14, haraken wrote:
> 
> Shall we add ALLOW_ONLY_INLINE_ALLOCATION?

Thanks, good idea. Done + added a comment on why.

https://codereview.chromium.org/176853004/diff/1/Source/core/html/forms/FileI...
File Source/core/html/forms/FileInputType.h (right):

https://codereview.chromium.org/176853004/diff/1/Source/core/html/forms/FileI...
Source/core/html/forms/FileInputType.h:80: RefPtrWillBePersistent<FileList>
m_fileList;
On 2014/02/24 13:49:14, haraken wrote:
> On 2014/02/24 13:12:43, sof wrote:
> > This persistent isn't promptly released & GCed on shutdown. Unless there are
> > mechanisms to make that happen, this CL may have to wait until Node-based
> > objects have been moved to Oilpan.
> 
> Why isn't FileInputType destructed on shutdown? Who retains the FileInputType?

I don't see FileInputType's dtor being called on any document having it. And
definitely not by the time the heap census is taken by assertEmpty().

https://codereview.chromium.org/176853004/diff/1/Source/core/xml/XMLHttpReque...
File Source/core/xml/XMLHttpRequest.h (right):

https://codereview.chromium.org/176853004/diff/1/Source/core/xml/XMLHttpReque...
Source/core/xml/XMLHttpRequest.h:227: RefPtrWillBePersistent<Stream>
m_responseStream;
On 2014/02/24 13:49:14, haraken wrote:
> 
> XMLHttpRequest is already on the heap. Thus these should be Members.

Oh, right - change here was started before that landed :)

https://codereview.chromium.org/176853004/diff/1/Source/modules/filesystem/DO...
File Source/modules/filesystem/DOMFileSystemSync.cpp (right):

https://codereview.chromium.org/176853004/diff/1/Source/modules/filesystem/DO...
Source/modules/filesystem/DOMFileSystemSync.cpp:175: return
result->m_file.get();
On 2014/02/24 13:49:14, haraken wrote:
> 
> Shall we add an implicit conversion from a persistent handle to a raw pointer?

I don't have the experience with the Oilpan types to say yay or nay (but I
haven't run into the need to explicitly 'unwrap' the persistent too often, I
think.)

https://codereview.chromium.org/176853004/diff/1/Source/modules/filesystem/Fi...
File Source/modules/filesystem/FileWriter.h (right):

https://codereview.chromium.org/176853004/diff/1/Source/modules/filesystem/Fi...
Source/modules/filesystem/FileWriter.h:51: class FileWriter FINAL : public
ScriptWrappable, public FileWriterBase, public ActiveDOMObject, public
EventTargetWithInlineData, public blink::WebFileWriterClient {
On 2014/02/24 13:49:14, haraken wrote:
> 
> I believe you're planning to move FileWriter to the heap in a follow-up :)

Yes, will put that up for review once the File API ready :)

https://codereview.chromium.org/176853004/diff/1/public/web/WebBlob.h
File public/web/WebBlob.h (right):

https://codereview.chromium.org/176853004/diff/1/public/web/WebBlob.h#newcode79
public/web/WebBlob.h:79: operator PassRefPtrWillBeRawPtr<WebCore::Blob>() const;
On 2014/02/24 13:49:14, haraken wrote:
> 
> Shall we remove the implicit conversion and instead implement getValue()?

It isn't used, so I dropped it instead.

https://codereview.chromium.org/176853004/diff/1/public/web/WebDOMError.h
File public/web/WebDOMError.h (right):

https://codereview.chromium.org/176853004/diff/1/public/web/WebDOMError.h#new...
public/web/WebDOMError.h:72: operator
PassRefPtrWillBeRawPtr<WebCore::DOMError>() const;
On 2014/02/24 13:49:14, haraken wrote:
> 
> Ditto.

Same; removed.

[haraken, 2014-02-25 01:37:46]

It looks like we need to address the FileInputType issue first. Once it's
resolved, I'm happy to review this CL :)

https://codereview.chromium.org/176853004/diff/1/Source/core/html/FormDataList.h
File Source/core/html/FormDataList.h (right):

https://codereview.chromium.org/176853004/diff/1/Source/core/html/FormDataLis...
Source/core/html/FormDataList.h:35: class Item {
On 2014/02/24 17:09:09, sof wrote:
> On 2014/02/24 13:49:14, haraken wrote:
> > 
> > Shall we add ALLOW_ONLY_INLINE_ALLOCATION?
> 
> Thanks, good idea. Done + added a comment on why.

I'd prefer to rename ALLOW_ONLY_INLINE_ALLOCATION to something better rather
than having a comment :)

ALLOW_ONLY_INLINE_ALLOCATION, STACK_ALLOCATED and DISALLOW_ALLOCATION are
confusing.

https://codereview.chromium.org/176853004/diff/1/Source/core/html/forms/FileI...
File Source/core/html/forms/FileInputType.h (right):

https://codereview.chromium.org/176853004/diff/1/Source/core/html/forms/FileI...
Source/core/html/forms/FileInputType.h:80: RefPtrWillBePersistent<FileList>
m_fileList;
On 2014/02/24 17:09:09, sof wrote:
> On 2014/02/24 13:49:14, haraken wrote:
> > On 2014/02/24 13:12:43, sof wrote:
> > > This persistent isn't promptly released & GCed on shutdown. Unless there
are
> > > mechanisms to make that happen, this CL may have to wait until Node-based
> > > objects have been moved to Oilpan.
> > 
> > Why isn't FileInputType destructed on shutdown? Who retains the
FileInputType?
> 
> I don't see FileInputType's dtor being called on any document having it. And
> definitely not by the time the heap census is taken by assertEmpty().
> 

Would you mind digging into the issue? This sounds like a real bug which we
should fix before doing the oilpanization.

[sof, 2014-02-26 08:35:35]

On 2014/02/25 01:37:46, haraken wrote:
> It looks like we need to address the FileInputType issue first. Once it's
> resolved, I'm happy to review this CL :)
> 
>
https://codereview.chromium.org/176853004/diff/1/Source/core/html/FormDataList.h
> File Source/core/html/FormDataList.h (right):
> 
>
https://codereview.chromium.org/176853004/diff/1/Source/core/html/FormDataLis...
> Source/core/html/FormDataList.h:35: class Item {
> On 2014/02/24 17:09:09, sof wrote:
> > On 2014/02/24 13:49:14, haraken wrote:
> > > 
> > > Shall we add ALLOW_ONLY_INLINE_ALLOCATION?
> > 
> > Thanks, good idea. Done + added a comment on why.
> 
> I'd prefer to rename ALLOW_ONLY_INLINE_ALLOCATION to something better rather
> than having a comment :)
> 
> ALLOW_ONLY_INLINE_ALLOCATION, STACK_ALLOCATED and DISALLOW_ALLOCATION are
> confusing.
> 
>
https://codereview.chromium.org/176853004/diff/1/Source/core/html/forms/FileI...
> File Source/core/html/forms/FileInputType.h (right):
> 
>
https://codereview.chromium.org/176853004/diff/1/Source/core/html/forms/FileI...
> Source/core/html/forms/FileInputType.h:80: RefPtrWillBePersistent<FileList>
> m_fileList;
> On 2014/02/24 17:09:09, sof wrote:
> > On 2014/02/24 13:49:14, haraken wrote:
> > > On 2014/02/24 13:12:43, sof wrote:
> > > > This persistent isn't promptly released & GCed on shutdown. Unless there
> are
> > > > mechanisms to make that happen, this CL may have to wait until
Node-based
> > > > objects have been moved to Oilpan.
> > > 
> > > Why isn't FileInputType destructed on shutdown? Who retains the
> FileInputType?
> > 
> > I don't see FileInputType's dtor being called on any document having it. And
> > definitely not by the time the heap census is taken by assertEmpty().
> > 
> 
> Would you mind digging into the issue? This sounds like a real bug which we
> should fix before doing the oilpanization.

On shutting down a WebView, we don't attempt to perform a final GC, so
e.g., V8HTMLDocument won't get around to decrementing the ref count, hence
nothing is released below it.

Is that (== a final shutdown GC) something we want to arrange for to always
happen in a debug setting?

[haraken, 2014-02-26 08:38:03]

> > Would you mind digging into the issue? This sounds like a real bug which we
> > should fix before doing the oilpanization.
> 
> On shutting down a WebView, we don't attempt to perform a final GC, so
> e.g., V8HTMLDocument won't get around to decrementing the ref count, hence
> nothing is released below it.
> 
> Is that (== a final shutdown GC) something we want to arrange for to always
> happen in a debug setting?

I think yes :)

In any setting, we should call the shutdown GC and make sure that the heap is
empty in the end.

[sof, 2014-02-26 08:46:38]

On 2014/02/26 08:38:03, haraken wrote:
> > > Would you mind digging into the issue? This sounds like a real bug which
we
> > > should fix before doing the oilpanization.
> > 
> > On shutting down a WebView, we don't attempt to perform a final GC, so
> > e.g., V8HTMLDocument won't get around to decrementing the ref count, hence
> > nothing is released below it.
> > 
> > Is that (== a final shutdown GC) something we want to arrange for to always
> > happen in a debug setting?
> 
> I think yes :)
> 
> In any setting, we should call the shutdown GC and make sure that the heap is
> empty in the end.

In the case of FileInputType, this would be a Persistent, so enforcing this
is-empty-heap assert on shutdown will require two shutdown GCs.

[haraken, 2014-02-26 08:52:57]

On 2014/02/26 08:46:38, sof wrote:
> On 2014/02/26 08:38:03, haraken wrote:
> > > > Would you mind digging into the issue? This sounds like a real bug which
> we
> > > > should fix before doing the oilpanization.
> > > 
> > > On shutting down a WebView, we don't attempt to perform a final GC, so
> > > e.g., V8HTMLDocument won't get around to decrementing the ref count, hence
> > > nothing is released below it.
> > > 
> > > Is that (== a final shutdown GC) something we want to arrange for to
always
> > > happen in a debug setting?
> > 
> > I think yes :)
> > 
> > In any setting, we should call the shutdown GC and make sure that the heap
is
> > empty in the end.
> 
> In the case of FileInputType, this would be a Persistent, so enforcing this
> is-empty-heap assert on shutdown will require two shutdown GCs.

Yes, that's expected.

But I'm a bit confused and have one question:

Slava: I thought ThreadState::cleanup is calling multiple GCs to break
persistent chains, but actually ThreadState::cleanup is calling only one GC. Why
can we break persistent chains with one GC?

[sof, 2014-02-26 10:23:14]

On 2014/02/26 08:52:57, haraken wrote:
> On 2014/02/26 08:46:38, sof wrote:
> > On 2014/02/26 08:38:03, haraken wrote:
> > > > > Would you mind digging into the issue? This sounds like a real bug
which
> > we
> > > > > should fix before doing the oilpanization.
> > > > 
> > > > On shutting down a WebView, we don't attempt to perform a final GC, so
> > > > e.g., V8HTMLDocument won't get around to decrementing the ref count,
hence
> > > > nothing is released below it.
> > > > 
> > > > Is that (== a final shutdown GC) something we want to arrange for to
> always
> > > > happen in a debug setting?
> > > 
> > > I think yes :)
> > > 
> > > In any setting, we should call the shutdown GC and make sure that the heap
> is
> > > empty in the end.
> > 
> > In the case of FileInputType, this would be a Persistent, so enforcing this
> > is-empty-heap assert on shutdown will require two shutdown GCs.
> 
> Yes, that's expected.
> 
> But I'm a bit confused and have one question:
> 
> Slava: I thought ThreadState::cleanup is calling multiple GCs to break
> persistent chains, but actually ThreadState::cleanup is calling only one GC.
Why
> can we break persistent chains with one GC?

Hmm, after rebasing & re-testing, I'm unable to reproduce the problem of having
a FileList persistent (from FileInputType) hanging around when the is-heap-empty
check ran on shutdown. (This is when running
fast/files/workers/worker-read-file-async.html, it was a dead solid repro
before.)

I don't immediately see what heap/ changes might have altered the behavior here,
but possibly https://codereview.chromium.org/169653003 was not in scope when
working on this initially. The persistent is swept out & its pointed-to FileList
object also.

So, as this is no longer a blocking issue, might we get this CL going? :)

[haraken, 2014-02-26 10:48:01]

> Hmm, after rebasing & re-testing, I'm unable to reproduce the problem of
having
> a FileList persistent (from FileInputType) hanging around when the
is-heap-empty
> check ran on shutdown. (This is when running
> fast/files/workers/worker-read-file-async.html, it was a dead solid repro
> before.)
> 
> I don't immediately see what heap/ changes might have altered the behavior
here,
> but possibly https://codereview.chromium.org/169653003 was not in scope when
> working on this initially. The persistent is swept out & its pointed-to
FileList
> object also.

hmm, sounds strange. The CL just implemented cross-thread persistent handles,
which shouldn't be related to this CL...

I do understand that it's a punishing work, but would you mind investigating the
issue a bit more? It's important to make sure that no persistent handles remain
in the worker thread heap when shutting down the worker thread.

[Mads Ager (chromium), 2014-02-26 11:00:27]

https://codereview.chromium.org/176853004/diff/210001/Source/core/clipboard/D...
File Source/core/clipboard/DataObjectItem.cpp (right):

https://codereview.chromium.org/176853004/diff/210001/Source/core/clipboard/D...
Source/core/clipboard/DataObjectItem.cpp:110: return m_file.get();
Just curious: do you remember why you need the .get() here? Because you need two
conversions (Member<T> -> T* and T* ->RawPtr) to go from Member to RawPtr?

https://codereview.chromium.org/176853004/diff/210001/Source/core/html/DOMFor...
File Source/core/html/DOMFormData.h (right):

https://codereview.chromium.org/176853004/diff/210001/Source/core/html/DOMFor...
Source/core/html/DOMFormData.h:49: class DOMFormData : public
RefCountedWillBeGarbageCollectedFinalized<DOMFormData>, public FormDataList,
public ScriptWrappable {
Hmm, this one doesn't have a trace method. Where is this used, I would expect a
compile-time error because the trace method cannot be found. Is it unused? Can
we just delete it?

https://codereview.chromium.org/176853004/diff/210001/Source/core/html/FormDa...
File Source/core/html/FormDataList.cpp (right):

https://codereview.chromium.org/176853004/diff/210001/Source/core/html/FormDa...
Source/core/html/FormDataList.cpp:43: m_items.append(Item(string));
It can no longer use the implicit conversion?

https://codereview.chromium.org/176853004/diff/210001/Source/core/html/FormDa...
File Source/core/html/FormDataList.h (right):

https://codereview.chromium.org/176853004/diff/210001/Source/core/html/FormDa...
Source/core/html/FormDataList.h:35: // This class is only allocated as part of
vectors, this class'
class's

https://codereview.chromium.org/176853004/diff/210001/Source/core/html/FormDa...
Source/core/html/FormDataList.h:37: class Item {
Until Gustav's change to automatically detect that Item value objects in
collections need tracing you will have to mark this as needing tracing. I think
adding this at the end of the file should do it:

template<>
struct WTF::NeedsTracing<WebCore::FormDataList::Item> {
    static const bool value = true;
};

I hope it will work for the nested class...

[sof, 2014-02-26 11:08:04]

On 2014/02/26 10:48:01, haraken wrote:
> > Hmm, after rebasing & re-testing, I'm unable to reproduce the problem of
> having
> > a FileList persistent (from FileInputType) hanging around when the
> is-heap-empty
> > check ran on shutdown. (This is when running
> > fast/files/workers/worker-read-file-async.html, it was a dead solid repro
> > before.)
> > 
> > I don't immediately see what heap/ changes might have altered the behavior
> here,
> > but possibly https://codereview.chromium.org/169653003 was not in scope when
> > working on this initially. The persistent is swept out & its pointed-to
> FileList
> > object also.
> 
> hmm, sounds strange. The CL just implemented cross-thread persistent handles,
> which shouldn't be related to this CL...
> 
> I do understand that it's a punishing work, but would you mind investigating
the
> issue a bit more? It's important to make sure that no persistent handles
remain
> in the worker thread heap when shutting down the worker thread.

Hmm, ok.

[sof, 2014-02-26 16:10:35]

https://codereview.chromium.org/176853004/diff/210001/Source/core/html/DOMFor...
File Source/core/html/DOMFormData.h (right):

https://codereview.chromium.org/176853004/diff/210001/Source/core/html/DOMFor...
Source/core/html/DOMFormData.h:49: class DOMFormData : public
RefCountedWillBeGarbageCollectedFinalized<DOMFormData>, public FormDataList,
public ScriptWrappable {
On 2014/02/26 11:00:29, Mads Ager (chromium) wrote:
> Hmm, this one doesn't have a trace method. Where is this used, I would expect
a
> compile-time error because the trace method cannot be found. Is it unused? Can
> we just delete it?

It isn't used as a Member nor Persistent on any other 'host object', but apart
from being able to directly construct it in scripts (new FormData()), form
submission code creates an instance while constructing the payload.

Will fix.

[sof, 2014-02-26 21:51:24]

https://codereview.chromium.org/176853004/diff/210001/Source/core/clipboard/D...
File Source/core/clipboard/DataObjectItem.cpp (right):

https://codereview.chromium.org/176853004/diff/210001/Source/core/clipboard/D...
Source/core/clipboard/DataObjectItem.cpp:110: return m_file.get();
On 2014/02/26 11:00:29, Mads Ager (chromium) wrote:
> Just curious: do you remember why you need the .get() here? Because you need
two
> conversions (Member<T> -> T* and T* ->RawPtr) to go from Member to RawPtr?

Yes, unbox m_file (either a RefPtr<T> or Member<T>) to enable the T* conversions
provided for either result type (PassRefPtr<T> or RawPtr<T>).

https://codereview.chromium.org/176853004/diff/210001/Source/core/html/FormDa...
File Source/core/html/FormDataList.cpp (right):

https://codereview.chromium.org/176853004/diff/210001/Source/core/html/FormDa...
Source/core/html/FormDataList.cpp:43: m_items.append(Item(string));
On 2014/02/26 11:00:29, Mads Ager (chromium) wrote:
> It can no longer use the implicit conversion?

It can; temporary change not fully backed out.

https://codereview.chromium.org/176853004/diff/210001/Source/core/html/FormDa...
File Source/core/html/FormDataList.h (right):

https://codereview.chromium.org/176853004/diff/210001/Source/core/html/FormDa...
Source/core/html/FormDataList.h:37: class Item {
On 2014/02/26 11:00:29, Mads Ager (chromium) wrote:
> Until Gustav's change to automatically detect that Item value objects in
> collections need tracing you will have to mark this as needing tracing. I
think
> adding this at the end of the file should do it:
> 
> template<>
> struct WTF::NeedsTracing<WebCore::FormDataList::Item> {
>     static const bool value = true;
> };
> 
> I hope it will work for the nested class...
> 

Done.

[sof, 2014-02-26 21:58:40]

https://codereview.chromium.org/176853004/diff/210001/Source/core/html/FormDa...
File Source/core/html/FormDataList.h (right):

https://codereview.chromium.org/176853004/diff/210001/Source/core/html/FormDa...
Source/core/html/FormDataList.h:37: class Item {
On 2014/02/26 21:51:25, sof wrote:
> On 2014/02/26 11:00:29, Mads Ager (chromium) wrote:
> > Until Gustav's change to automatically detect that Item value objects in
> > collections need tracing you will have to mark this as needing tracing. I
> think
> > adding this at the end of the file should do it:
> > 
> > template<>
> > struct WTF::NeedsTracing<WebCore::FormDataList::Item> {
> >     static const bool value = true;
> > };
> > 
> > I hope it will work for the nested class...
> > 
> 
> Done.

Forgot to add..I'm intruiged by how that can/will be done :)

[haraken, 2014-02-27 01:19:28]

Thanks for being persistent!

I'm happy to stamp LG once the FileInputType's lifetime issue is clarified.

https://codereview.chromium.org/176853004/diff/250001/Source/core/dom/DOMError.h
File Source/core/dom/DOMError.h (right):

https://codereview.chromium.org/176853004/diff/250001/Source/core/dom/DOMErro...
Source/core/dom/DOMError.h:63: void trace(Visitor*) { }

This should be virtual.

https://codereview.chromium.org/176853004/diff/250001/Source/core/fileapi/Fil...
File Source/core/fileapi/FileError.h (right):

https://codereview.chromium.org/176853004/diff/250001/Source/core/fileapi/Fil...
Source/core/fileapi/FileError.h:44: class FileError : public DOMError {

Let's implement virtual trace() methods to all subclasses of DOMError.

https://codereview.chromium.org/176853004/diff/250001/Source/core/html/FormDa...
File Source/core/html/FormDataList.h (right):

https://codereview.chromium.org/176853004/diff/250001/Source/core/html/FormDa...
Source/core/html/FormDataList.h:37: // See also its WTF::NeedsTracing
specialization below.

Nit: I'd drop this comment, as we don't add this kind of comment to other
ALLOW_ONLY_INLINE_ALLOCATION()s. (Rather, I welcome a better name for the
macro:-)

https://codereview.chromium.org/176853004/diff/250001/public/web/WebBlob.h
File public/web/WebBlob.h (right):

https://codereview.chromium.org/176853004/diff/250001/public/web/WebBlob.h#ne...
public/web/WebBlob.h:40: #if INSIDE_BLINK

BLINK_IMPLEMENTATION (since you're using BLINK_IMPLEMENTATION below) ?

[sof, 2014-02-27 06:48:06]

https://codereview.chromium.org/176853004/diff/250001/Source/core/dom/DOMError.h
File Source/core/dom/DOMError.h (right):

https://codereview.chromium.org/176853004/diff/250001/Source/core/dom/DOMErro...
Source/core/dom/DOMError.h:63: void trace(Visitor*) { }
On 2014/02/27 01:19:29, haraken wrote:
> 
> This should be virtual.

I'm not sure about that given its nature. I raised a question on this earlier --
https://codereview.chromium.org/176853004/diff/1/Source/core/fileapi/FileErro...

https://codereview.chromium.org/176853004/diff/250001/Source/core/html/FormDa...
File Source/core/html/FormDataList.h (right):

https://codereview.chromium.org/176853004/diff/250001/Source/core/html/FormDa...
Source/core/html/FormDataList.h:37: // See also its WTF::NeedsTracing
specialization below.
On 2014/02/27 01:19:29, haraken wrote:
> 
> Nit: I'd drop this comment, as we don't add this kind of comment to other
> ALLOW_ONLY_INLINE_ALLOCATION()s. (Rather, I welcome a better name for the
> macro:-)

alright, but there is one next to the use in CSSGradientColorStop which this
took its inspiration from.

https://codereview.chromium.org/176853004/diff/250001/public/web/WebBlob.h
File public/web/WebBlob.h (right):

https://codereview.chromium.org/176853004/diff/250001/public/web/WebBlob.h#ne...
public/web/WebBlob.h:40: #if INSIDE_BLINK
On 2014/02/27 01:19:29, haraken wrote:
> 
> BLINK_IMPLEMENTATION (since you're using BLINK_IMPLEMENTATION below) ?

Yes, thanks.

[haraken, 2014-02-27 06:53:21]

https://codereview.chromium.org/176853004/diff/1/Source/core/fileapi/FileError.h
File Source/core/fileapi/FileError.h (right):

https://codereview.chromium.org/176853004/diff/1/Source/core/fileapi/FileErro...
Source/core/fileapi/FileError.h:44: class FileError : public DOMError {
On 2014/02/24 17:09:09, sof wrote:
> On 2014/02/24 13:49:14, haraken wrote:
> > 
> > Let's override a trace() method in all subclasses of a garbage-collected
class
> > (even if the class does not have any Member).
> 
> Even if the class, like here, doesn't have any other virtual methods?

IMO the answer is yes unless the class is not memory-sensitive. In other words,
if the class is memory-sensitive, we should have traceAfterDispatch(). If the
class is not memory-sensitive, we should have virtual trace().

[sof, 2014-02-27 07:16:35]

https://codereview.chromium.org/176853004/diff/1/Source/core/fileapi/FileError.h
File Source/core/fileapi/FileError.h (right):

https://codereview.chromium.org/176853004/diff/1/Source/core/fileapi/FileErro...
Source/core/fileapi/FileError.h:44: class FileError : public DOMError {
On 2014/02/27 06:53:22, haraken wrote:
> On 2014/02/24 17:09:09, sof wrote:
> > On 2014/02/24 13:49:14, haraken wrote:
> > > 
> > > Let's override a trace() method in all subclasses of a garbage-collected
> class
> > > (even if the class does not have any Member).
> > 
> > Even if the class, like here, doesn't have any other virtual methods?
> 
> IMO the answer is yes unless the class is not memory-sensitive. In other
words,
> if the class is memory-sensitive, we should have traceAfterDispatch(). If the
> class is not memory-sensitive, we should have virtual trace().

Subclassing DOMError isn't something that's commonly done, so I would
additionally preach common sense in that regard :)

i.e., FileError is legacy and something that will eventually be deprecated &
removed. It is no longer in the spec &  use counter data for it can be found
here http://www.chromestatus.com/metrics/feature/timeline/popularity/126

[haraken, 2014-02-27 07:20:11]

https://codereview.chromium.org/176853004/diff/1/Source/core/fileapi/FileError.h
File Source/core/fileapi/FileError.h (right):

https://codereview.chromium.org/176853004/diff/1/Source/core/fileapi/FileErro...
Source/core/fileapi/FileError.h:44: class FileError : public DOMError {
On 2014/02/27 07:16:36, sof wrote:
> On 2014/02/27 06:53:22, haraken wrote:
> > On 2014/02/24 17:09:09, sof wrote:
> > > On 2014/02/24 13:49:14, haraken wrote:
> > > > 
> > > > Let's override a trace() method in all subclasses of a garbage-collected
> > class
> > > > (even if the class does not have any Member).
> > > 
> > > Even if the class, like here, doesn't have any other virtual methods?
> > 
> > IMO the answer is yes unless the class is not memory-sensitive. In other
> words,
> > if the class is memory-sensitive, we should have traceAfterDispatch(). If
the
> > class is not memory-sensitive, we should have virtual trace().
> 
> Subclassing DOMError isn't something that's commonly done, so I would
> additionally preach common sense in that regard :)
> 
> i.e., FileError is legacy and something that will eventually be deprecated &
> removed. It is no longer in the spec &  use counter data for it can be found
> here http://www.chromestatus.com/metrics/feature/timeline/popularity/126

Makes sense, then let's go this way. Once we add a Member to FileError, we will
be asserted by Ian's plugin :)

[sof, 2014-02-27 12:27:25]

On 2014/02/27 01:19:28, haraken wrote:
> Thanks for being persistent!
> 
> I'm happy to stamp LG once the FileInputType's lifetime issue is clarified.

With https://codereview.chromium.org/181823004 in place, spending time debugging
what changed & didn't wrt Persistents seems even less worthwhile.

[haraken, 2014-02-27 12:29:54]

On 2014/02/27 12:27:25, sof wrote:
> On 2014/02/27 01:19:28, haraken wrote:
> > Thanks for being persistent!
> > 
> > I'm happy to stamp LG once the FileInputType's lifetime issue is clarified.
> 
> With https://codereview.chromium.org/181823004 in place, spending time
debugging
> what changed & didn't wrt Persistents seems even less worthwhile.

ah, that's plausible... Previously we were calling the final GC only once, so it
was not guaranteed that all persistent chains get broken.

LGTM. Let's land this CL and see how it goes.

[sof, 2014-02-27 12:49:51]

On 2014/02/27 12:29:54, haraken wrote:
> On 2014/02/27 12:27:25, sof wrote:
> > On 2014/02/27 01:19:28, haraken wrote:
> > > Thanks for being persistent!
> > > 
> > > I'm happy to stamp LG once the FileInputType's lifetime issue is
clarified.
> > 
> > With https://codereview.chromium.org/181823004 in place, spending time
> debugging
> > what changed & didn't wrt Persistents seems even less worthwhile.
> 
> ah, that's plausible... Previously we were calling the final GC only once, so
it
> was not guaranteed that all persistent chains get broken.
> 
> LGTM. Let's land this CL and see how it goes.

Great, thanks very much for the help (&persistence :) ).

Will rebase & test locally first, of course. (i did run into the assert in
WorkerThread::workerThread() this morning while testing fast/files/workers, but
perhaps that has been addressed on trunk by now. EventTarget change related,
perhaps.)

[sof, 2014-02-27 17:04:01]

This CL touches Source/web/ and platform/ ever so slightly -- could Cc:ed owners
please take a look?

[tkent, 2014-02-28 00:09:57]

The patch is too large.  Can you split this into multiple pieces?
e.g.
 - DOMError and FileError
 - Blob and File
 - ...

[sof, 2014-02-28 06:11:38]

On 2014/02/28 00:09:57, tkent wrote:
> The patch is too large.  Can you split this into multiple pieces?
> e.g.
>  - DOMError and FileError
>  - Blob and File
>  - ...

I'd prefer not.

[sof, 2014-03-01 14:44:05]

https://codereview.chromium.org/176853004/diff/290001/Source/core/events/Mess...
File Source/core/events/MessageEvent.cpp (right):

https://codereview.chromium.org/176853004/diff/290001/Source/core/events/Mess...
Source/core/events/MessageEvent.cpp:190: visitor->trace(m_dataAsBlob);
When rebasing this CL to have it cleanly apply with current trunk, the changes
here met with the support for heap-allocated Events.

Hence, MessageEvent has now switched its Blob reference to be a Member rather
than a Persistent. Please double check.

[tkent, 2014-03-03 07:52:37]

lgtm. But please do not land this now because oilpan bots are on fire.

https://codereview.chromium.org/176853004/diff/1/Source/heap/ThreadState.h
File Source/heap/ThreadState.h (right):

https://codereview.chromium.org/176853004/diff/1/Source/heap/ThreadState.h#ne...
Source/heap/ThreadState.h:551: USED_FROM_MULTIPLE_THREADS(Blob);
Please rebase.

https://codereview.chromium.org/176853004/diff/310001/Source/core/fileapi/Fil...
File Source/core/fileapi/FileError.idl (right):

https://codereview.chromium.org/176853004/diff/310001/Source/core/fileapi/Fil...
Source/core/fileapi/FileError.idl:31: interface FileError : DOMError {
nit: This change is unnecessary.

https://codereview.chromium.org/176853004/diff/310001/Source/modules/mediastr...
File Source/modules/mediastream/RTCDataChannel.idl (right):

https://codereview.chromium.org/176853004/diff/310001/Source/modules/mediastr...
Source/modules/mediastream/RTCDataChannel.idl:39: 
nit: this change is unnecessary.

[sof, 2014-03-03 10:14:29]

On 2014/03/03 07:52:37, tkent wrote:
> lgtm. But please do not land this now because oilpan bots are on fire.
> 

Thanks, waiting for the Event revert to land. Some smaller changes required here
because of that.

(The CL has been compiled with the Blink GC plugin enabled, btw.)

[sof, 2014-03-04 08:33:47]

Rebased the branch; which also meant changing MessageEvent back to keeping a
Persistent to its Blob rather than Member (with Oilpan enabled.)

https://codereview.chromium.org/176853004/diff/1/Source/heap/ThreadState.h
File Source/heap/ThreadState.h (right):

https://codereview.chromium.org/176853004/diff/1/Source/heap/ThreadState.h#ne...
Source/heap/ThreadState.h:551: USED_FROM_MULTIPLE_THREADS(Blob);
On 2014/03/03 07:52:38, tkent wrote:
> Please rebase.

Done (with pleasure :) )

https://codereview.chromium.org/176853004/diff/310001/Source/core/fileapi/Fil...
File Source/core/fileapi/FileError.idl (right):

https://codereview.chromium.org/176853004/diff/310001/Source/core/fileapi/Fil...
Source/core/fileapi/FileError.idl:31: interface FileError : DOMError {
On 2014/03/03 07:52:38, tkent wrote:
> nit: This change is unnecessary.

Reverted back to previous.

https://codereview.chromium.org/176853004/diff/310001/Source/modules/mediastr...
File Source/modules/mediastream/RTCDataChannel.idl (right):

https://codereview.chromium.org/176853004/diff/310001/Source/modules/mediastr...
Source/modules/mediastream/RTCDataChannel.idl:39: 
On 2014/03/03 07:52:38, tkent wrote:
> nit: this change is unnecessary.

Reverted whitespace change.

[tkent, 2014-03-04 08:55:35]

https://codereview.chromium.org/176853004/diff/1/Source/heap/ThreadState.h
File Source/heap/ThreadState.h (right):

https://codereview.chromium.org/176853004/diff/1/Source/heap/ThreadState.h#ne...
Source/heap/ThreadState.h:551: USED_FROM_MULTIPLE_THREADS(Blob);
On 2014/03/04 08:33:49, sof wrote:
> On 2014/03/03 07:52:38, tkent wrote:
> > Please rebase.
> 
> Done (with pleasure :) )

Thanks.  Please go ahead.

[sof, 2014-03-04 09:41:41]

On 2014/03/04 08:55:35, tkent wrote:
> https://codereview.chromium.org/176853004/diff/1/Source/heap/ThreadState.h
> File Source/heap/ThreadState.h (right):
> 
>
https://codereview.chromium.org/176853004/diff/1/Source/heap/ThreadState.h#ne...
> Source/heap/ThreadState.h:551: USED_FROM_MULTIPLE_THREADS(Blob);
> On 2014/03/04 08:33:49, sof wrote:
> > On 2014/03/03 07:52:38, tkent wrote:
> > > Please rebase.
> > 
> > Done (with pleasure :) )
> 
> Thanks.  Please go ahead.

Great, thanks. I've confirmed that it merges fine with
https://codereview.chromium.org/178663004/, so sending it along in parallel.

[sof, 2014-03-04 09:41:57]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-03-04 09:42:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjornf@opera.com/176853004/330001

[sof, 2014-03-05 10:12:12]

The CQ bit was unchecked by sigbjornf@opera.com

[sof, 2014-03-05 10:13:07]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-03-05 10:13:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjornf@opera.com/176853004/330001

[commit-bot: I haz the power, 2014-03-05 10:15:23]

Change committed as 168460