Bug 159255 – Purge PassRefPtr in Modules/webdatabase

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Gyuyoung Kim]

SSIA.

[Gyuyoung Kim]

Created attachment 282334 [details]
Patch

[Alex Christensen]

Comment on attachment 282334 [details]
Patch

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

> Source/WebCore/Modules/webdatabase/Database.cpp:249
> -        PassRefPtr<ScriptExecutionContext> passedContext = m_scriptExecutionContext.release();
> +        RefPtr<ScriptExecutionContext> passedContext = WTFMove(m_scriptExecutionContext);
>          passedContext->postTask({ScriptExecutionContext::Task::CleanupTask, [passedContext] (ScriptExecutionContext& context) {

c++14 allows you to do the WTFMove in the capture, like this:
passedContext = WTFMove(m_scriptExecutionContext)

> Source/WebCore/Modules/webdatabase/Database.h:128
> +    Database(RefPtr<DatabaseContext>&, const String& name, const String& expectedVersion, const String& displayName, unsigned long estimatedSize);

RefPtr<DatabaseContext>&&

[Gyuyoung Kim]

Created attachment 282394 [details]
Patch

[Gyuyoung Kim]

(In reply to comment #2)
> Comment on attachment 282334 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=282334&action=review
> 
> > Source/WebCore/Modules/webdatabase/Database.cpp:249
> > -        PassRefPtr<ScriptExecutionContext> passedContext = m_scriptExecutionContext.release();
> > +        RefPtr<ScriptExecutionContext> passedContext = WTFMove(m_scriptExecutionContext);
> >          passedContext->postTask({ScriptExecutionContext::Task::CleanupTask, [passedContext] (ScriptExecutionContext& context) {
> 
> c++14 allows you to do the WTFMove in the capture, like this:
> passedContext = WTFMove(m_scriptExecutionContext)
> 
> > Source/WebCore/Modules/webdatabase/Database.h:128
> > +    Database(RefPtr<DatabaseContext>&, const String& name, const String& expectedVersion, const String& displayName, unsigned long estimatedSize);
> 
> RefPtr<DatabaseContext>&&


I see. Fixed all. Thanks~

[Benjamin Poulain]

Comment on attachment 282394 [details]
Patch

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

> Source/WebCore/Modules/webdatabase/SQLStatement.h:62
> +    RefPtr<SQLError> sqlError() const;
> +    SQLResultSet* sqlResultSet() const;

This is an inconsistent design.
Why is one returned by RefPTR and the other by pointer?

Please change this before landing unless you have a rationale.

[Gyuyoung Kim]

Created attachment 282414 [details]
Patch for landing

[Gyuyoung Kim]

(In reply to comment #5)
> Comment on attachment 282394 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=282394&action=review
> 
> > Source/WebCore/Modules/webdatabase/SQLStatement.h:62
> > +    RefPtr<SQLError> sqlError() const;
> > +    SQLResultSet* sqlResultSet() const;
> 
> This is an inconsistent design.
> Why is one returned by RefPTR and the other by pointer?
> 
> Please change this before landing unless you have a rationale.

I thought sqlError() was referenced by other variables. For example, 


void SQLTransactionBackend::openTransactionAndPreflight() {

...
 m_transactionError = m_wrapper->sqlError();

However it looks m_transactionError just calls member functions, not referenced or is passed to other variables. So it seems to me that sqlError() can be changed as well.

[WebKit Commit Bot]

Comment on attachment 282414 [details]
Patch for landing

Clearing flags on attachment: 282414

Committed r202676: <http://trac.webkit.org/changeset/202676>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

This change appears to have caused storage/websql test crashes

See crashlogs from mac-wk1 here:
https://build.webkit.org/results/Apple%20El%20Capitan%20Debug%20WK1%20(Tests)/r202693%20(6322)/results.html

[WebKit Commit Bot]

Re-opened since this is blocked by bug 159314

[Gyuyoung Kim]

Created attachment 282758 [details]
Patch

[Gyuyoung Kim]

Below lambda function caused the test crash on mac-wk1. However I don't know how to fix this issue yet. So I'd like to prepare a new patch after landing this patch. 

In Database.cpp,

Database::~Database()
{
...
        PassRefPtr<ScriptExecutionContext> passedContext = m_scriptExecutionContext.release();
        passedContext->postTask({ScriptExecutionContext::Task::CleanupTask, [passedContext] (ScriptExecutionContext& context) {
            ASSERT_UNUSED(context, &context == passedContext);
            RefPtr<ScriptExecutionContext> scriptExecutionContext(passedContext);
        }});
...

[Gyuyoung Kim]

Ryan, Darin, could you take a look ? Last patch doesn't cause the test crash anymore.

[WebKit Commit Bot]

Comment on attachment 282758 [details]
Patch

Clearing flags on attachment: 282758

Committed r203146: <http://trac.webkit.org/changeset/203146>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.