Issue 314333002: Enable Oilpan by default in modules/filesystem/

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+246 lines, -215 lines)			Patch
M	Source/bindings/scripts/v8_callback_interface.py	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/bindings/v8/SerializedScriptValue.cpp	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/modules/filesystem/DOMFilePath.h	View		1 chunk	+0 lines, -1 line	0 comments	Download
M	Source/modules/filesystem/DOMFileSystem.h	View	1 2	6 chunks	+42 lines, -6 lines	0 comments	Download
M	Source/modules/filesystem/DOMFileSystem.cpp	View	1 2 3 4	4 chunks	+11 lines, -11 lines	0 comments	Download
M	Source/modules/filesystem/DOMFileSystem.idl	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/modules/filesystem/DOMFileSystemBase.h	View		3 chunks	+2 lines, -3 lines	0 comments	Download
M	Source/modules/filesystem/DOMFileSystemBase.cpp	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/modules/filesystem/DOMFileSystemSync.h	View	1 2 3	1 chunk	+5 lines, -5 lines	0 comments	Download
M	Source/modules/filesystem/DOMFileSystemSync.cpp	View	1 2 3	4 chunks	+8 lines, -8 lines	0 comments	Download
M	Source/modules/filesystem/DOMFileSystemSync.idl	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/modules/filesystem/DataTransferItemFileSystem.h	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/modules/filesystem/DataTransferItemFileSystem.cpp	View	1 2 3	2 chunks	+5 lines, -5 lines	0 comments	Download
M	Source/modules/filesystem/DirectoryEntry.h	View		3 chunks	+4 lines, -5 lines	0 comments	Download
M	Source/modules/filesystem/DirectoryEntry.cpp	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/modules/filesystem/DirectoryEntrySync.h	View		2 chunks	+6 lines, -7 lines	0 comments	Download
M	Source/modules/filesystem/DirectoryEntrySync.cpp	View		1 chunk	+6 lines, -6 lines	0 comments	Download
M	Source/modules/filesystem/DirectoryReader.h	View		2 chunks	+6 lines, -8 lines	0 comments	Download
M	Source/modules/filesystem/DirectoryReader.cpp	View	1 2 3	4 chunks	+7 lines, -7 lines	0 comments	Download
M	Source/modules/filesystem/DirectoryReader.idl	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/modules/filesystem/DirectoryReaderBase.h	View		2 chunks	+3 lines, -4 lines	0 comments	Download
M	Source/modules/filesystem/DirectoryReaderSync.h	View		3 chunks	+4 lines, -6 lines	0 comments	Download
M	Source/modules/filesystem/DirectoryReaderSync.cpp	View	1 2 3	3 chunks	+5 lines, -5 lines	0 comments	Download
M	Source/modules/filesystem/DirectoryReaderSync.idl	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/modules/filesystem/DraggedIsolatedFileSystem.h	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/modules/filesystem/EntriesCallback.h	View		1 chunk	+1 line, -2 lines	0 comments	Download
M	Source/modules/filesystem/Entry.h	View		2 chunks	+3 lines, -4 lines	0 comments	Download
M	Source/modules/filesystem/Entry.cpp	View		2 chunks	+5 lines, -5 lines	0 comments	Download
M	Source/modules/filesystem/Entry.idl	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/modules/filesystem/EntryBase.h	View		3 chunks	+3 lines, -4 lines	0 comments	Download
M	Source/modules/filesystem/EntryBase.cpp	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/modules/filesystem/EntrySync.h	View		2 chunks	+6 lines, -7 lines	0 comments	Download
M	Source/modules/filesystem/EntrySync.cpp	View		2 chunks	+8 lines, -8 lines	0 comments	Download
M	Source/modules/filesystem/EntrySync.idl	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/modules/filesystem/FileEntry.h	View		2 chunks	+3 lines, -3 lines	0 comments	Download
M	Source/modules/filesystem/FileEntry.cpp	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/modules/filesystem/FileEntrySync.h	View	1 2 3	2 chunks	+4 lines, -5 lines	0 comments	Download
M	Source/modules/filesystem/FileEntrySync.cpp	View	1 2 3	2 chunks	+2 lines, -2 lines	0 comments	Download
M	Source/modules/filesystem/FileSystemCallbacks.h	View	1 2 3 4	5 chunks	+16 lines, -13 lines	0 comments	Download
M	Source/modules/filesystem/FileSystemCallbacks.cpp	View	1	8 chunks	+25 lines, -14 lines	0 comments	Download
M	Source/modules/filesystem/FileWriter.h	View	1 2 3 4	1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/modules/filesystem/FileWriter.cpp	View	1 2 3 4	1 chunk	+3 lines, -3 lines	0 comments	Download
M	Source/modules/filesystem/FileWriter.idl	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/modules/filesystem/FileWriterBase.h	View		1 chunk	+1 line, -2 lines	0 comments	Download
M	Source/modules/filesystem/FileWriterSync.h	View	1 2 3	1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/modules/filesystem/FileWriterSync.idl	View	1 2 3	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/modules/filesystem/HTMLInputElementFileSystem.cpp	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/modules/filesystem/InspectorFileSystemAgent.cpp	View		2 chunks	+2 lines, -2 lines	0 comments	Download
M	Source/modules/filesystem/InspectorFrontendHostFileSystem.h	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/modules/filesystem/InspectorFrontendHostFileSystem.cpp	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/modules/filesystem/Metadata.h	View		1 chunk	+5 lines, -6 lines	0 comments	Download
M	Source/modules/filesystem/Metadata.idl	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/modules/filesystem/SyncCallbackHelper.h	View	1 2 3	2 chunks	+5 lines, -5 lines	0 comments	Download
M	Source/modules/filesystem/WorkerGlobalScopeFileSystem.h	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/modules/filesystem/WorkerGlobalScopeFileSystem.cpp	View	1 2 3	3 chunks	+7 lines, -10 lines	0 comments	Download
M	Source/web/WebDOMFileSystem.cpp	View	1 2 3 4 5 6	2 chunks	+3 lines, -3 lines	0 comments	Download
M	public/web/WebDOMFileSystem.h	View	1 2 3 4 5 6	1 chunk	+2 lines, -2 lines	0 comments	Download

Messages

Total messages: 15 (0 generated)

Expand Messages | Collapse Messages

sof

Please take a look. Verified with clang & the GC plugin with enable_oilpan={0,1}

6 years, 6 months ago (2014-06-06 15:25:57 UTC) #1

haraken

https://codereview.chromium.org/314333002/diff/40001/Source/modules/filesystem/DOMFileSystem.h File Source/modules/filesystem/DOMFileSystem.h (right): https://codereview.chromium.org/314333002/diff/40001/Source/modules/filesystem/DOMFileSystem.h#newcode125 Source/modules/filesystem/DOMFileSystem.h:125: RefPtrWillBePersistent<CBArg> m_callbackArg; Just to confirm: Not related to CL, ...

6 years, 6 months ago (2014-06-07 08:11:04 UTC) #2

sof

6 years, 6 months ago (2014-06-07 13:18:54 UTC) #3

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

https://codereview.chromium.org/314333002/diff/40001/Source/modules/filesyste...
Source/modules/filesystem/DOMFileSystem.h:125: RefPtrWillBePersistent<CBArg>
m_callbackArg;
On 2014/06/07 08:11:04, haraken wrote:
> 
> Just to confirm: Not related to CL, but doesn't this need to be
> RefPtrWillBeCrossThreadPersistent?

See comment at line 70 of this file? (they're assumed not to cross threads.)

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

https://codereview.chromium.org/314333002/diff/40001/Source/modules/filesyste...
Source/modules/filesystem/DataTransferItemFileSystem.cpp:54: return nullptr;
On 2014/06/07 08:11:04, haraken wrote:
> 
> return 0;
> 
> I think you'll hit a compile error in g++.

None of the trybots have reported problems, but no need to run the risk either
with older versions.

I guess we'll get to change them back again later to nullptr :)

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

https://codereview.chromium.org/314333002/diff/40001/Source/modules/filesyste...
Source/modules/filesystem/DirectoryReader.cpp:43:
EntriesCallbackHelper(DirectoryReader* reader)
On 2014/06/07 08:11:04, haraken wrote:
> 
> Add explicit.

Done.

https://codereview.chromium.org/314333002/diff/40001/Source/modules/filesyste...
Source/modules/filesystem/DirectoryReader.cpp:60:
ErrorCallbackHelper(DirectoryReader* reader)
On 2014/06/07 08:11:04, haraken wrote:
> 
> Add explicit.

Done.

https://codereview.chromium.org/314333002/diff/40001/Source/modules/filesyste...
Source/modules/filesystem/DirectoryReader.cpp:93:
filesystem()->scheduleCallback(errorCallback,
PassRefPtrWillBeRawPtr<FileError>(m_error.get()));
On 2014/06/07 08:11:04, haraken wrote:
> 
> I'm curious why this can't be just 'm_error'.

Let's see. scheduleCallback() has three function template:

 static void scheduleCallback(ExecutionContext*,PassOwnPtr<CB>,
PassRefWillBeRawPtr<CBArg>);
 static void scheduleCallback(ExecutionContext*,PassOwnPtr<CB>, CBArg*);
 static void scheduleCallback(ExecutionContext*,PassOwnPtr<CB>, const CBArg&);

if we pass m_error (of type RefPtrWillBeMember<FileError>), then in Oilpan mode:

 Member<FileError> matches the 3rd version (as no implicit conversions happen
prior to template matching), with CBArg = Member<FileError>

The function template then instantiates a Task template, which has a call to
handleError(FileError*) within it, e.g., m_callback->handleError(m_error) is
what it expands to.

The implicit conversion "operator T*()" over Member then kicks in and everything
is fine.

For the non-Oilpan case, m_error is of type RefPtr<FileError>, so you end up
with m_callback->handleError(m_error) and ask the compiler to convert a
RefPtr<FileError> to a FileError*. No implicit conversion exist over RefPtr<>
for that, hence compilation failure.

If instead you say, "let's just use m_error.get() then", you will resolve to the
2nd scheduleCallback() version above. This runs into trouble for the non-Oilpan
case, as it keeps the callback argument in a Persistent<> (to support bare
Oilpan pointers), which you cannot put a non-GCed pointer into (FileError.)

Which is why this ends up as PassRefPtrWillBeRawPtr<FileError>(m_error.get()) to
force the use of the first overload. It doesn't work without the .get(), as
there is RawPtr(Member<T>) constructor.

Does that explain things?

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

https://codereview.chromium.org/314333002/diff/40001/Source/modules/filesyste...
Source/modules/filesystem/DirectoryReaderSync.cpp:47:
EntriesCallbackHelper(DirectoryReaderSync* reader)
On 2014/06/07 08:11:04, haraken wrote:
> 
> Add explicit.

Done.

https://codereview.chromium.org/314333002/diff/40001/Source/modules/filesyste...
Source/modules/filesystem/DirectoryReaderSync.cpp:67:
ErrorCallbackHelper(DirectoryReaderSync* reader)
On 2014/06/07 08:11:04, haraken wrote:
> 
> Add explicit.

Done.

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

https://codereview.chromium.org/314333002/diff/40001/Source/modules/filesyste...
Source/modules/filesystem/DraggedIsolatedFileSystem.h:43: class
DraggedIsolatedFileSystem FINAL : public
NoBaseWillBeGarbageCollectedFinalized<DraggedIsolatedFileSystem>, public
WillBeHeapSupplement<DataObject> {
On 2014/06/07 08:11:04, haraken wrote:
> 
> Is there any reason why you didn't enable oilpan by default for
> DraggedIsolatedFileSystem?

DataObject hasn't been put on the heap by default yet. Consequently, its
Supplements cannot be on the heap by default either.

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

https://codereview.chromium.org/314333002/diff/40001/Source/modules/filesyste...
Source/modules/filesystem/FileSystemCallbacks.h:79:
Persistent<DOMFileSystemBase> m_fileSystem;
On 2014/06/07 08:11:04, haraken wrote:
> 
> Are you sure that persistent handles in this file don't need to be
> CrossThreadPersistent?

Same comment as elsewhere regarding CrossThreadPersistent.

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

https://codereview.chromium.org/314333002/diff/40001/Source/modules/filesyste...
Source/modules/filesystem/FileWriter.cpp:47: PassRefPtrWillBeRawPtr<FileWriter>
FileWriter::create(ExecutionContext* context)
On 2014/06/07 08:11:04, haraken wrote:
> 
> This should be FileWriter*.
> 
> Since FileWriter is still RefCountedGarbageCollected, compilers won't complain
> about remaining RefPtr<FileWriter> :)

I'm not yet making EventTarget-derived objects live on the heap by default. Are
we ready for that yet with Oilpan disabled?

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

https://codereview.chromium.org/314333002/diff/40001/Source/modules/filesyste...
Source/modules/filesystem/FileWriterSync.h:47: static
PassRefPtrWillBeRawPtr<FileWriterSync> create()
On 2014/06/07 08:11:04, haraken wrote:
> 
> Ditto. FileWriterSync*.
> 
> Let's manually grep RefPtrs to RefCountedGarbageCollected objects and remove
> them.

That will work as it is not EventTarget-derived. Done.

haraken

https://codereview.chromium.org/314333002/diff/40001/Source/modules/filesystem/DirectoryReader.cpp File Source/modules/filesystem/DirectoryReader.cpp (right): https://codereview.chromium.org/314333002/diff/40001/Source/modules/filesystem/DirectoryReader.cpp#newcode93 Source/modules/filesystem/DirectoryReader.cpp:93: filesystem()->scheduleCallback(errorCallback, PassRefPtrWillBeRawPtr<FileError>(m_error.get())); On 2014/06/07 13:18:53, sof wrote: > On ...

6 years, 6 months ago (2014-06-07 16:45:27 UTC) #4

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

https://codereview.chromium.org/314333002/diff/40001/Source/modules/filesyste...
Source/modules/filesystem/DirectoryReader.cpp:93:
filesystem()->scheduleCallback(errorCallback,
PassRefPtrWillBeRawPtr<FileError>(m_error.get()));
On 2014/06/07 13:18:53, sof wrote:
> On 2014/06/07 08:11:04, haraken wrote:
> > 
> > I'm curious why this can't be just 'm_error'.
> 
> Let's see. scheduleCallback() has three function template:
> 
>  static void scheduleCallback(ExecutionContext*,PassOwnPtr<CB>,
> PassRefWillBeRawPtr<CBArg>);
>  static void scheduleCallback(ExecutionContext*,PassOwnPtr<CB>, CBArg*);
>  static void scheduleCallback(ExecutionContext*,PassOwnPtr<CB>, const CBArg&);
> 
> if we pass m_error (of type RefPtrWillBeMember<FileError>), then in Oilpan
mode:
> 
>  Member<FileError> matches the 3rd version (as no implicit conversions happen
> prior to template matching), with CBArg = Member<FileError>
> 
> The function template then instantiates a Task template, which has a call to
> handleError(FileError*) within it, e.g., m_callback->handleError(m_error) is
> what it expands to.
> 
> The implicit conversion "operator T*()" over Member then kicks in and
everything
> is fine.
> 
> For the non-Oilpan case, m_error is of type RefPtr<FileError>, so you end up
> with m_callback->handleError(m_error) and ask the compiler to convert a
> RefPtr<FileError> to a FileError*. No implicit conversion exist over RefPtr<>
> for that, hence compilation failure.
> 
> If instead you say, "let's just use m_error.get() then", you will resolve to
the
> 2nd scheduleCallback() version above. This runs into trouble for the
non-Oilpan
> case, as it keeps the callback argument in a Persistent<> (to support bare
> Oilpan pointers), which you cannot put a non-GCed pointer into (FileError.)
> 
> Which is why this ends up as PassRefPtrWillBeRawPtr<FileError>(m_error.get())
to
> force the use of the first overload. It doesn't work without the .get(), as
> there is RawPtr(Member<T>) constructor.
> 
> Does that explain things?

Thanks for the details, makes sense.

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

https://codereview.chromium.org/314333002/diff/40001/Source/modules/filesyste...
Source/modules/filesystem/FileWriter.cpp:47: PassRefPtrWillBeRawPtr<FileWriter>
FileWriter::create(ExecutionContext* context)
On 2014/06/07 13:18:53, sof wrote:
> On 2014/06/07 08:11:04, haraken wrote:
> > 
> > This should be FileWriter*.
> > 
> > Since FileWriter is still RefCountedGarbageCollected, compilers won't
complain
> > about remaining RefPtr<FileWriter> :)
> 
> I'm not yet making EventTarget-derived objects live on the heap by default.
Are
> we ready for that yet with Oilpan disabled?

I think yes. We need to keep FileWriter being RefCountedGarbageCollected, but we
can remove RefPtr<FileWriter> from the code base. RefPtrs to FileWriter objects
should be allowed only when they are treated as EventTargets.

For example, IDBDatabase is an EventTarget but IDBDatabase::create returns a raw
pointer.

sof

https://codereview.chromium.org/314333002/diff/40001/Source/modules/filesystem/FileWriter.cpp File Source/modules/filesystem/FileWriter.cpp (right): https://codereview.chromium.org/314333002/diff/40001/Source/modules/filesystem/FileWriter.cpp#newcode47 Source/modules/filesystem/FileWriter.cpp:47: PassRefPtrWillBeRawPtr<FileWriter> FileWriter::create(ExecutionContext* context) On 2014/06/07 16:45:27, haraken wrote: > ...

6 years, 6 months ago (2014-06-08 16:34:34 UTC) #5

haraken

tzik@: Would it be OK to enable oilpan by default for filesystem/ ? (kinuko-san is ...

6 years, 6 months ago (2014-06-09 00:40:58 UTC) #7

sof

Source/web/ + public/ approval remains, but there is the same issue there as the one ...

6 years, 6 months ago (2014-06-09 15:24:31 UTC) #9

tkent

https://codereview.chromium.org/314333002/diff/100001/Source/web/WebDOMFileSystem.cpp File Source/web/WebDOMFileSystem.cpp (right): https://codereview.chromium.org/314333002/diff/100001/Source/web/WebDOMFileSystem.cpp#newcode142 Source/web/WebDOMFileSystem.cpp:142: WebDOMFileSystem::WebDOMFileSystem(const DOMFileSystem* domFileSystem) Please remove |const|. We should not ...

6 years, 6 months ago (2014-06-10 03:14:23 UTC) #10

sof

6 years, 6 months ago (2014-06-10 06:14:35 UTC) #11

commit-bot: I haz the power

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

6 years, 6 months ago (2014-06-10 06:26:02 UTC) #14

Message was sent while issue was closed.

Change committed as 175867

Expand Messages | Collapse Messages

Issue 314333002: Enable Oilpan by default in modules/filesystem/ (Closed)

Description

Patch Set 1 #

Patch Set 2 : Non-oilpan compile fixes (GC plugin) #

Patch Set 3 : Tidy up #

Patch Set 4 : Make FileWriterSync [GarbageCollected] #

Patch Set 5 : No longer use transition type for FileWriter constructor either #

Patch Set 6 : Rebased + be const-consistent in WebDOMFileSystem constructor #

Patch Set 7 : Remove consts from conversion ctor + copy assignment op #

Messages