Oilpan: add transition types to FileSystem APIs.

Oilpan: add transition types to FileSystem APIs.

R=
BUG=340522
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=168897

[sof, 2014-03-06 10:38:25]

Please take a look.

[haraken, 2014-03-06 15:09:30]

On 2014/03/06 10:38:25, sof wrote:
> Please take a look.

Let me take a look tomorrow :)

[sof, 2014-03-06 15:16:54]

On 2014/03/06 15:09:30, haraken wrote:
> On 2014/03/06 10:38:25, sof wrote:
> > Please take a look.
> 
> Let me take a look tomorrow :)

Great, that works :)

fwiw, I didn't come across anything non-regular for these interfaces.

[haraken, 2014-03-07 08:24:35]

Sorry for the review delay! Overall the CL looks good.

This CL contains a bunch of non-trivial destructors. Let's make sure that each
destructor is never touching on-heap objects.

https://codereview.chromium.org/188503002/diff/20001/Source/modules/filesyste...
File Source/modules/filesystem/DOMFileSystemBase.h (right):

https://codereview.chromium.org/188503002/diff/20001/Source/modules/filesyste...
Source/modules/filesystem/DOMFileSystemBase.h:124: ExecutionContext* m_context;

Are you sure that this raw pointer is safe? There is no code to clear m_context
when the ExecutionContext is destroyed.

https://codereview.chromium.org/188503002/diff/20001/Source/modules/filesyste...
File Source/modules/filesystem/DOMFileSystemSync.cpp (right):

https://codereview.chromium.org/188503002/diff/20001/Source/modules/filesyste...
Source/modules/filesystem/DOMFileSystemSync.cpp:109: friend class
RefCountedWillBeGarbageCollected<CreateFileResult>;

Not related to this CL, this class is mis-designed. This class has not only
public members but also a friend class..

https://codereview.chromium.org/188503002/diff/20001/Source/modules/filesyste...
File Source/modules/filesystem/DirectoryEntrySync.h (right):

https://codereview.chromium.org/188503002/diff/20001/Source/modules/filesyste...
Source/modules/filesystem/DirectoryEntrySync.h:46: class DirectoryEntrySync
FINAL : public EntrySync {

Shall we add a trace() method to all GarbageCollected sub-classes? The same
comment for other places in this CL.

https://codereview.chromium.org/188503002/diff/20001/Source/modules/filesyste...
File Source/modules/filesystem/DirectoryReaderSync.h (right):

https://codereview.chromium.org/188503002/diff/20001/Source/modules/filesyste...
Source/modules/filesystem/DirectoryReaderSync.h:47: typedef
WillBeHeapVector<RefPtrWillBeMember<EntrySync> > EntrySyncVector;

EntrySyncVector => EntrySyncHeapVector (to make the underlying data structure
clearer)?

https://codereview.chromium.org/188503002/diff/20001/Source/modules/filesyste...
File Source/modules/filesystem/EntriesCallback.h (right):

https://codereview.chromium.org/188503002/diff/20001/Source/modules/filesyste...
Source/modules/filesystem/EntriesCallback.h:40: typedef
WillBeHeapVector<RefPtrWillBeMember<Entry> > EntryVector;

EntryVector => EntryHeapVector

https://codereview.chromium.org/188503002/diff/20001/Source/modules/filesyste...
File Source/modules/filesystem/FileSystemCallbacks.cpp (right):

https://codereview.chromium.org/188503002/diff/20001/Source/modules/filesyste...
Source/modules/filesystem/FileSystemCallbacks.cpp:231:
m_successCallback->handleEvent(writer.get());

I'd write this as:

  m_successCallback->handleEvent(m_fileWriter.get());
  m_fileWriter = nullptr;

https://codereview.chromium.org/188503002/diff/20001/Source/modules/filesyste...
File Source/modules/filesystem/FileSystemCallbacks.h (right):

https://codereview.chromium.org/188503002/diff/20001/Source/modules/filesyste...
Source/modules/filesystem/FileSystemCallbacks.h:96:
WillBePersistentHeapVector<RefPtrWillBeMember<Entry> > m_entries;

I'm just curious: If you forget to replace EntryVector with
WillBePersistentHeapVector<RefPtrWillBeMember<Entry>>, how can you notice the
error?

https://codereview.chromium.org/188503002/diff/20001/Source/modules/filesyste...
File Source/modules/filesystem/FileWriter.cpp (right):

https://codereview.chromium.org/188503002/diff/20001/Source/modules/filesyste...
Source/modules/filesystem/FileWriter.cpp:284: m_blobBeingWritten.clear();

This looks dangerous.

FileWriter's destructor calls doOperation() and the doOperation() touches 
|m_blobBeingWritten|, which is on the heap.

https://codereview.chromium.org/188503002/diff/20001/Source/modules/filesyste...
File Source/modules/filesystem/FileWriter.h (right):

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

FileWriterBase should be the left-most class. I'd be happy if this error is
detected by the plugin :)

https://codereview.chromium.org/188503002/diff/20001/Source/modules/filesyste...
File Source/modules/filesystem/SyncCallbackHelper.h (right):

https://codereview.chromium.org/188503002/diff/20001/Source/modules/filesyste...
Source/modules/filesystem/SyncCallbackHelper.h:55: typedef
PassRefPtrWillBeRawPtr<ResultType> ReturnType;

This is used by only one place. Shall we avoid using typedef?

https://codereview.chromium.org/188503002/diff/20001/Source/modules/filesyste...
Source/modules/filesystem/SyncCallbackHelper.h:56: typedef
RefPtrWillBeRawPtr<ResultType> StorageType;

This is unused. Shall we remove?

https://codereview.chromium.org/188503002/diff/20001/Source/modules/filesyste...
Source/modules/filesystem/SyncCallbackHelper.h:67: typedef EntrySyncVector
StorageType;

Looks like these two typedefs are unused. Shall we remove?

[sof, 2014-03-07 09:37:35]

https://codereview.chromium.org/188503002/diff/20001/Source/modules/filesyste...
File Source/modules/filesystem/DOMFileSystemBase.h (right):

https://codereview.chromium.org/188503002/diff/20001/Source/modules/filesyste...
Source/modules/filesystem/DOMFileSystemBase.h:124: ExecutionContext* m_context;
On 2014/03/07 08:24:35, haraken wrote:
> 
> Are you sure that this raw pointer is safe? There is no code to clear
m_context
> when the ExecutionContext is destroyed.

It is used to gain access to the SecurityOrigin when serializing a FileSystem
URL & that only. Not squeaky tidy, but I don't think there is a non-overlap in
lifetimes to be concerned with.

https://codereview.chromium.org/188503002/diff/20001/Source/modules/filesyste...
File Source/modules/filesystem/DirectoryEntrySync.h (right):

https://codereview.chromium.org/188503002/diff/20001/Source/modules/filesyste...
Source/modules/filesystem/DirectoryEntrySync.h:46: class DirectoryEntrySync
FINAL : public EntrySync {
On 2014/03/07 08:24:35, haraken wrote:
> 
> Shall we add a trace() method to all GarbageCollected sub-classes? The same
> comment for other places in this CL.

I don't think it adds much value, given the class's only function --
FileEntrySync&DirectoryEntrySync are just here to get the kind/type of the
EntrySync right. Ditto for FileEntry and DirectoryEntry. That's why I didn't add
them.

https://codereview.chromium.org/188503002/diff/20001/Source/modules/filesyste...
File Source/modules/filesystem/DirectoryReaderSync.h (right):

https://codereview.chromium.org/188503002/diff/20001/Source/modules/filesyste...
Source/modules/filesystem/DirectoryReaderSync.h:47: typedef
WillBeHeapVector<RefPtrWillBeMember<EntrySync> > EntrySyncVector;
On 2014/03/07 08:24:35, haraken wrote:
> 
> EntrySyncVector => EntrySyncHeapVector (to make the underlying data structure
> clearer)?

Done.

https://codereview.chromium.org/188503002/diff/20001/Source/modules/filesyste...
File Source/modules/filesystem/EntriesCallback.h (right):

https://codereview.chromium.org/188503002/diff/20001/Source/modules/filesyste...
Source/modules/filesystem/EntriesCallback.h:40: typedef
WillBeHeapVector<RefPtrWillBeMember<Entry> > EntryVector;
On 2014/03/07 08:24:35, haraken wrote:
> 
> EntryVector => EntryHeapVector

Done.

https://codereview.chromium.org/188503002/diff/20001/Source/modules/filesyste...
File Source/modules/filesystem/FileSystemCallbacks.cpp (right):

https://codereview.chromium.org/188503002/diff/20001/Source/modules/filesyste...
Source/modules/filesystem/FileSystemCallbacks.cpp:231:
m_successCallback->handleEvent(writer.get());
On 2014/03/07 08:24:35, haraken wrote:
> 
> I'd write this as:
> 
>   m_successCallback->handleEvent(m_fileWriter.get());
>   m_fileWriter = nullptr;

Done.

https://codereview.chromium.org/188503002/diff/20001/Source/modules/filesyste...
File Source/modules/filesystem/FileSystemCallbacks.h (right):

https://codereview.chromium.org/188503002/diff/20001/Source/modules/filesyste...
Source/modules/filesystem/FileSystemCallbacks.h:96:
WillBePersistentHeapVector<RefPtrWillBeMember<Entry> > m_entries;
On 2014/03/07 08:24:35, haraken wrote:
> 
> I'm just curious: If you forget to replace EntryVector with
> WillBePersistentHeapVector<RefPtrWillBeMember<Entry>>, how can you notice the
> error?

I couldn't -- this is something the GC plugin pointed out :)

(typedefs that hide a transition type are something to keep an extra eye was one
of my conclusions from that.)

https://codereview.chromium.org/188503002/diff/20001/Source/modules/filesyste...
File Source/modules/filesystem/FileWriter.cpp (right):

https://codereview.chromium.org/188503002/diff/20001/Source/modules/filesyste...
Source/modules/filesystem/FileWriter.cpp:284: m_blobBeingWritten.clear();
On 2014/03/07 08:24:35, haraken wrote:
> 
> This looks dangerous.
> 
> FileWriter's destructor calls doOperation() and the doOperation() touches 
> |m_blobBeingWritten|, which is on the heap.
> 

Very well spotted, but isn't this OK, really? i.e., it clears the Member & isn't
touching the object.

https://codereview.chromium.org/188503002/diff/20001/Source/modules/filesyste...
File Source/modules/filesystem/FileWriter.h (right):

https://codereview.chromium.org/188503002/diff/20001/Source/modules/filesyste...
Source/modules/filesystem/FileWriter.h:51: class FileWriter FINAL : public
ScriptWrappable, public FileWriterBase, public ActiveDOMObject, public
EventTargetWithInlineData, public blink::WebFileWriterClient {
On 2014/03/07 08:24:35, haraken wrote:
> 
> FileWriterBase should be the left-most class. I'd be happy if this error is
> detected by the plugin :)

Fixed; it didn't detect this (version built sometime last evening.)

https://codereview.chromium.org/188503002/diff/20001/Source/modules/filesyste...
File Source/modules/filesystem/SyncCallbackHelper.h (right):

https://codereview.chromium.org/188503002/diff/20001/Source/modules/filesyste...
Source/modules/filesystem/SyncCallbackHelper.h:55: typedef
PassRefPtrWillBeRawPtr<ResultType> ReturnType;
On 2014/03/07 08:24:35, haraken wrote:
> 
> This is used by only one place. Shall we avoid using typedef?

It is used later by derived classes to gain access storage & return types.

https://codereview.chromium.org/188503002/diff/20001/Source/modules/filesyste...
Source/modules/filesystem/SyncCallbackHelper.h:56: typedef
RefPtrWillBeRawPtr<ResultType> StorageType;
On 2014/03/07 08:24:35, haraken wrote:
> 
> This is unused. Shall we remove?

Same, elsewhere used.

https://codereview.chromium.org/188503002/diff/20001/Source/modules/filesyste...
Source/modules/filesystem/SyncCallbackHelper.h:67: typedef EntrySyncVector
StorageType;
On 2014/03/07 08:24:35, haraken wrote:
> 
> Looks like these two typedefs are unused. Shall we remove?

Ditto, elsewhere used.

[haraken, 2014-03-10 01:55:25]

LGTM (if you add trace() methods to sub-classes.)

I might want to have one more oilpan reviewer.

https://codereview.chromium.org/188503002/diff/20001/Source/modules/filesyste...
File Source/modules/filesystem/DirectoryEntrySync.h (right):

https://codereview.chromium.org/188503002/diff/20001/Source/modules/filesyste...
Source/modules/filesystem/DirectoryEntrySync.h:46: class DirectoryEntrySync
FINAL : public EntrySync {
On 2014/03/07 09:37:36, sof wrote:
> On 2014/03/07 08:24:35, haraken wrote:
> > 
> > Shall we add a trace() method to all GarbageCollected sub-classes? The same
> > comment for other places in this CL.
> 
> I don't think it adds much value, given the class's only function --
> FileEntrySync&DirectoryEntrySync are just here to get the kind/type of the
> EntrySync right. Ditto for FileEntry and DirectoryEntry. That's why I didn't
add
> them.

I'd prefer to add (and in fact we have added) trace() methods to all
garbage-collected classes even if the classes don't have any members. This is
for programming consistency. Instead of making developers add trace() methods
every time they add new members, it would be simpler and less error-prone to add
trace() methods when we move the class hierarchy to the heap.

https://codereview.chromium.org/188503002/diff/40001/Source/modules/filesyste...
File Source/modules/filesystem/FileSystemCallbacks.cpp (right):

https://codereview.chromium.org/188503002/diff/40001/Source/modules/filesyste...
Source/modules/filesystem/FileSystemCallbacks.cpp:131:
m_successCallback->handleEvent(m_entries);

Here you're passing PersistentHeapVector<Entry> to HeapVector<Entry>&. This will
work as expected, right?

Probably Erik might know better.

https://codereview.chromium.org/188503002/diff/40001/Source/modules/filesyste...
File Source/modules/filesystem/FileWriter.cpp (right):

https://codereview.chromium.org/188503002/diff/40001/Source/modules/filesyste...
Source/modules/filesystem/FileWriter.cpp:69: FileWriter::~FileWriter()

Just to confirm: Is this destructor safe? This destructor is doing a lot of
non-trivial things. As far as I see, this destructor looks safe since it won't
touch any on-heap objects that can be collected in the same GC cycle.

https://codereview.chromium.org/188503002/diff/40001/Source/modules/filesyste...
File Source/modules/filesystem/SyncCallbackHelper.h (right):

https://codereview.chromium.org/188503002/diff/40001/Source/modules/filesyste...
Source/modules/filesystem/SyncCallbackHelper.h:54: struct HelperResultType {

Shall we add STACK_ALLOCATED or DISALLOW_ALLOCATION? (I don't know how this
struct is going to be used in the future.)

https://codereview.chromium.org/188503002/diff/40001/Source/modules/filesyste...
Source/modules/filesystem/SyncCallbackHelper.h:65: struct
HelperResultType<EntrySyncHeapVector, const EntryHeapVector&> {

Ditto.

[sof, 2014-03-10 10:01:23]

https://codereview.chromium.org/188503002/diff/20001/Source/modules/filesyste...
File Source/modules/filesystem/DirectoryEntrySync.h (right):

https://codereview.chromium.org/188503002/diff/20001/Source/modules/filesyste...
Source/modules/filesystem/DirectoryEntrySync.h:46: class DirectoryEntrySync
FINAL : public EntrySync {
On 2014/03/10 01:55:26, haraken wrote:
> On 2014/03/07 09:37:36, sof wrote:
> > On 2014/03/07 08:24:35, haraken wrote:
> > > 
> > > Shall we add a trace() method to all GarbageCollected sub-classes? The
same
> > > comment for other places in this CL.
> > 
> > I don't think it adds much value, given the class's only function --
> > FileEntrySync&DirectoryEntrySync are just here to get the kind/type of the
> > EntrySync right. Ditto for FileEntry and DirectoryEntry. That's why I didn't
> add
> > them.
> 
> I'd prefer to add (and in fact we have added) trace() methods to all
> garbage-collected classes even if the classes don't have any members. This is
> for programming consistency. Instead of making developers add trace() methods
> every time they add new members, it would be simpler and less error-prone to
add
> trace() methods when we move the class hierarchy to the heap.

ok, added the methods here & will follow that.

(It doesn't apply here, but if we can be judicious about adding "empty" trace()s
to classes having no other virtuals, I'd prefer that.)

https://codereview.chromium.org/188503002/diff/40001/Source/modules/filesyste...
File Source/modules/filesystem/FileSystemCallbacks.cpp (right):

https://codereview.chromium.org/188503002/diff/40001/Source/modules/filesyste...
Source/modules/filesystem/FileSystemCallbacks.cpp:131:
m_successCallback->handleEvent(m_entries);
On 2014/03/10 01:55:26, haraken wrote:
> 
> Here you're passing PersistentHeapVector<Entry> to HeapVector<Entry>&. This
will
> work as expected, right?
> 
> Probably Erik might know better.

I would expect that to work, multiple references to the heap vector shouldn't
pose a problem.

(The handleEvent() methods all/both copy the incoming vector right now. This can
be avoided once Oilpan becomes default.)

https://codereview.chromium.org/188503002/diff/40001/Source/modules/filesyste...
File Source/modules/filesystem/FileWriter.cpp (right):

https://codereview.chromium.org/188503002/diff/40001/Source/modules/filesyste...
Source/modules/filesystem/FileWriter.cpp:69: FileWriter::~FileWriter()
On 2014/03/10 01:55:26, haraken wrote:
> 
> Just to confirm: Is this destructor safe? This destructor is doing a lot of
> non-trivial things. As far as I see, this destructor looks safe since it won't
> touch any on-heap objects that can be collected in the same GC cycle.

Re-verified that it is safe as it currently is.

Not a future-proof property.. would it be worth adding a comment?

https://codereview.chromium.org/188503002/diff/40001/Source/modules/filesyste...
File Source/modules/filesystem/SyncCallbackHelper.h (right):

https://codereview.chromium.org/188503002/diff/40001/Source/modules/filesyste...
Source/modules/filesystem/SyncCallbackHelper.h:54: struct HelperResultType {
On 2014/03/10 01:55:26, haraken wrote:
> 
> Shall we add STACK_ALLOCATED or DISALLOW_ALLOCATION? (I don't know how this
> struct is going to be used in the future.)

Done (DISALLOW_ALLOCATION.)

[haraken, 2014-03-10 10:19:34]

https://codereview.chromium.org/188503002/diff/40001/Source/modules/filesyste...
File Source/modules/filesystem/FileWriter.cpp (right):

https://codereview.chromium.org/188503002/diff/40001/Source/modules/filesyste...
Source/modules/filesystem/FileWriter.cpp:69: FileWriter::~FileWriter()
On 2014/03/10 10:01:23, sof wrote:
> On 2014/03/10 01:55:26, haraken wrote:
> > 
> > Just to confirm: Is this destructor safe? This destructor is doing a lot of
> > non-trivial things. As far as I see, this destructor looks safe since it
won't
> > touch any on-heap objects that can be collected in the same GC cycle.
> 
> Re-verified that it is safe as it currently is.
> 
> Not a future-proof property.. would it be worth adding a comment?

Won't be needed :) Once we have the ASAN build in the future, we will be
asserted if we touch on-heap objects inside destructors.

[sof, 2014-03-10 12:48:27]

tkent, jochen: approve of web/ changes?

[tkent, 2014-03-11 03:36:21]

web/ lgtm.

[sof, 2014-03-11 06:22:48]

haraken: wait for this CL to be double-checked?

[haraken, 2014-03-11 06:47:17]

On 2014/03/11 06:22:48, sof wrote:
> haraken: wait for this CL to be double-checked?

LGTM.

[sof, 2014-03-11 06:52:41]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-03-11 06:52:49]

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

[commit-bot: I haz the power, 2014-03-11 08:18:32]

Change committed as 168897