Bug 59234 – CrossThreadCopier should not have a default specialization for raw pointers

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Dmitry Lomov]

Raw pointers are dangerous to pass cross-thread. Special annotations should be required.

[Dmitry Lomov]

Created attachment 90764 [details]
Proposed patch

Proposed patch - let me know what you think.
As a minimum, we should give a good thought to the names (AllowCrossThreadAccess and AllowExtendedLifetime).

[David Levin]

Comment on attachment 90764 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=90764&action=review

In general, I think this is a great change!

 I think we should think about the names a bit more.

Also putting the extended life time stuff into the CrossThreadCopier.h doesn't quite fit with theme of that file.

Some other minor comments below.

> Source/WebCore/ChangeLog:4
> +

Bug title should be here.

> Source/WebCore/fileapi/FileStreamProxy.cpp:140
> +    fileThread()->postTask(

Please undo this change since nothing is being done here except reformatting.

> Source/WebCore/fileapi/FileStreamProxy.cpp:153
> +    fileThread()->postTask(

Ditto.

> Source/WebCore/fileapi/FileStreamProxy.cpp:166
> +    fileThread()->postTask(

Ditto.

> Source/WebCore/fileapi/FileStreamProxy.cpp:204
> +        createFileThreadTask(this,

Ditto.

> Source/WebCore/fileapi/FileStreamProxy.cpp:223
> +        createFileThreadTask(this,

Ditto.

> Source/WebCore/storage/IDBObjectStoreBackendImpl.cpp:90
> +          createCallbackTask(&IDBObjectStoreBackendImpl::getInternal,

Ditto.

> Source/WebCore/storage/IDBObjectStoreBackendImpl.cpp:139
> +            createCallbackTask(&IDBObjectStoreBackendImpl::putInternal,

Ditto.

> Source/WebCore/storage/IDBObjectStoreBackendImpl.cpp:289
> +            createCallbackTask(&IDBObjectStoreBackendImpl::deleteInternal,

Ditto.

> Source/WebCore/storage/IDBObjectStoreBackendImpl.cpp:325
> +              createCallbackTask(&IDBObjectStoreBackendImpl::clearInternal,

Ditto and many other places. (I'm tired of putting in ditto :) ).

[Dmitry Lomov]

(In reply to comment #2)
> (From update of attachment 90764 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=90764&action=review
> 
> In general, I think this is a great change!
> 
>  I think we should think about the names a bit more.
> 
> Also putting the extended life time stuff into the CrossThreadCopier.h doesn't quite fit with theme of that file.

The real problem is not that we have something related to extended lifetime in CrossThreadCopier, but the fact that CrossThreadCopier is required where no cross-thread communication happen! 

Viz, in FileReader and in SQLCallbackWrapper, the posted tasks are guaranteed to execute on the _same_ thread, and implementation throughout relies on this fact heavily (mutates objects without any synchronization). If we statically type our threading assumptions, we should distinguish between true cross-thread (i.e. parallel) access and "co-routine style" access. I think they should be different kinds of CallbackTasks, coroutine one just passing everything via RefPtr.

[Dmitry Lomov]

(In reply to comment #2)
> (From update of attachment 90764 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=90764&action=review
> 
> In general, I think this is a great change!
> 
>  I think we should think about the names a bit more.
> 
> Also putting the extended life time stuff into the CrossThreadCopier.h doesn't quite fit with theme of that file.

The real problem is not that we have something related to extended lifetime in CrossThreadCopier, but the fact that CrossThreadCopier is required where no cross-thread communication happen! 

Viz, in FileReader and in SQLCallbackWrapper, the posted tasks are guaranteed to execute on the _same_ thread, and implementation throughout relies on this fact heavily (mutates objects without any synchronization). If we statically type our threading assumptions, we should distinguish between true cross-thread (i.e. parallel) access and "co-routine style" access. I think they should be different kinds of CallbackTasks, coroutine one just passing everything via RefPtr.

[Dmitry Lomov]

Created attachment 90930 [details]
Proposed patch: second iteration

Code review feedback addressed.

There is one real (and quite horrible) issue that this patch uncovered - direct your attention to changes in  IDBObjectStoreBackendImpl.cpp.
In two places, the code created a 
   RefPtr<Transaction> transactionPtr = transaction;
where transaction is Transaction*, but then passed a transaction, and not a transactionPtr, to a scheduled task. Why this does not die a horrible death I have no idea.
I would venture a suggestion that, besides a lax implementation of CrossThreadCopier, a horrendous line length that pushed arguments to scheduleTask way beyond an editor view port, was a contributing factor to this bug.

On a subject of AllowExtendedLifetime: my suggestion would be - let us keep AllowExtendedLifetime as an eyesore - it is something that should not be there. ScriptExecutionContext should have a special method that only schedules tasks on current thread.

[WebKit Review Bot]

Attachment 90930 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/storage/IDBObjectStoreBackendImpl.cpp:320:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dmitry Lomov]

Created attachment 90931 [details]
Style issues fixed

Style issues fixed, but check previous patch for extra comments

[Eric Seidel (no email)]

Comment on attachment 90931 [details]
Style issues fixed

View in context: https://bugs.webkit.org/attachment.cgi?id=90931&action=review

> Source/WebCore/fileapi/FileReader.cpp:133
> +        scriptExecutionContext()->postTask(
> +            createCallbackTask(&delayedStart, AllowExtendedLifetime(this)));

webkit has no column limit.

> Source/WebCore/platform/CrossThreadCopier.h:142
> +    template<typename T> struct AllowExtendedLifetimeWrapper {
> +      public:
> +        explicit AllowExtendedLifetimeWrapper(T* value) : m_value(value) { }
> +        T* value() const { return m_value; }
> +      private:
> +        T* m_value;
> +    };

4 space indent.

[Dmitry Lomov]

Created attachment 91195 [details]
Style feedback

More style feedback

[David Levin]

Comment on attachment 91195 [details]
Style feedback

View in context: https://bugs.webkit.org/attachment.cgi?id=91195&action=review

Some initial comments. I need to look a little more at this though.

> Source/WebCore/platform/CrossThreadCopier.h:128
> +        static Type copy(const AllowCrossThreadAccessWrapper<T>& r) { return r.value(); }

Avoid single letter variables in WebKit (other places too).

> Source/WebCore/platform/CrossThreadCopier.h:134
> +    };

No ; needed here.

> Source/WebCore/platform/CrossThreadCopier.h:136
> +    template<typename T> struct AllowExtendedLifetimeWrapper {

AllowAccessLater? (feels like it mirrors the name AllowCrossThreadAccessWrapper better).

> Source/WebCore/platform/CrossThreadCopier.h:144
> +    template<typename T> struct CrossThreadCopierBase<false, false, AllowExtendedLifetimeWrapper<T> > {

Should add a FIXME about moving this out of this header file.

> Source/WebCore/platform/CrossThreadCopier.h:152
> +    };

No ; needed here.

> Source/WebCore/storage/IDBObjectStoreBackendImpl.cpp:91
> +

Best not to do misc whitespace changes.

> Source/WebCore/storage/IDBObjectStoreBackendImpl.cpp:457
> +void IDBObjectStoreBackendImpl::openCursor(PassRefPtr<IDBKeyRange> prpRange, unsigned short direction, PassRefPtr<IDBCallbacks> prpCallbacks, IDBTransactionBackendInterface* transaction, ExceptionCode& ec)

I like the old naming better because IDBTransactionBackendInterface* is a pointer but RefPtr<IDBTransactionBackendInterface> is not.

> Source/WebCore/websockets/WorkerThreadableWebSocketChannel.cpp:251
> +                          AllowCrossThreadAccess(thisPtr), 

indenting is off.

[David Levin]

(In reply to comment #10) 
> I like the old naming better because IDBTransactionBackendInterface* is a pointer but RefPtr<IDBTransactionBackendInterface> is not.

I take this back and I have no other comments.

[Dmitry Lomov]

Created attachment 91389 [details]
CR feedback addressed

[Dmitry Lomov]

Created attachment 91390 [details]
CR feedback addressed

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 91390 [details]:

java/lc3/JSObject/ToObject-001.html bug 53091 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 91390 [details]
CR feedback addressed

Clearing flags on attachment: 91390

Committed r85165: <http://trac.webkit.org/changeset/85165>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/85165 might have broken Qt Linux Release
The following tests are not passing:
fast/events/mouseout-dead-node.html