Bug 56350 – Fix crash caused by Invalid call to destroyActiveDOMObject during stopActiveDOMObjects

• Component: New Bugs
• Status: RESOLVED FIXED

[David Grogan]

Fix crash caused by Invalid call to destroyActiveDOMObject during stopActiveDOMObjects

[David Grogan]

Created attachment 85750 [details]
Patch

[David Grogan]

Created attachment 85752 [details]
Patch

[WebKit Review Bot]

Attachment 85750 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8175439

[David Grogan]

chrome bug and review:
http://crbug.com/75264
http://codereview.chromium.org/6677034/

[David Grogan]

This fixes the crash but I'm not confident that memory isn't leaked and that I did the unregister thing right.

[Jeremy Orlow]

Comment on attachment 85752 [details]
Patch

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

close

> Source/WebCore/ChangeLog:7
> +

Please add some more detail and explain why it only affects multi-process.

> Source/WebCore/storage/IDBDatabase.h:118
> +    RefPtr<IDBDatabaseCallbacks> m_databaseCallbacks;

Save the impl here.

> Source/WebCore/storage/IDBDatabaseCallbacks.h:36
> +class IDBDatabase;

don't add

> Source/WebCore/storage/IDBDatabaseCallbacks.h:43
> +    virtual void unRegisterDatabase(IDBDatabase*) { ASSERT_NOT_REACHED(); };

don't add

> Source/WebCore/storage/IDBDatabaseCallbacksImpl.h:42
> +    virtual ~IDBDatabaseCallbacksImpl() { }

Don't inline.

> Source/WebCore/storage/IDBDatabaseCallbacksImpl.h:48
> +    IDBDatabaseCallbacksImpl(IDBDatabase*);

newline after

explain why this needs to be weak in comment?

[David Grogan]

Created attachment 85827 [details]
Patch

[David Grogan]

Comment on attachment 85752 [details]
Patch

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

>> Source/WebCore/ChangeLog:7
>> +
> 
> Please add some more detail and explain why it only affects multi-process.

done

>> Source/WebCore/storage/IDBDatabase.h:118
>> +    RefPtr<IDBDatabaseCallbacks> m_databaseCallbacks;
> 
> Save the impl here.

done

>> Source/WebCore/storage/IDBDatabaseCallbacks.h:36
>> +class IDBDatabase;
> 
> don't add

done

>> Source/WebCore/storage/IDBDatabaseCallbacks.h:43
>> +    virtual void unRegisterDatabase(IDBDatabase*) { ASSERT_NOT_REACHED(); };
> 
> don't add

done

>> Source/WebCore/storage/IDBDatabaseCallbacksImpl.h:42
>> +    virtual ~IDBDatabaseCallbacksImpl() { }
> 
> Don't inline.

done

>> Source/WebCore/storage/IDBDatabaseCallbacksImpl.h:48
>> +    IDBDatabaseCallbacksImpl(IDBDatabase*);
> 
> newline after
> 
> explain why this needs to be weak in comment?

done

[Jeremy Orlow]

Comment on attachment 85827 [details]
Patch

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

r=me

> Source/WebCore/storage/IDBDatabaseCallbacksImpl.h:45
> +    void unRegisterDatabase(IDBDatabase*);

How do we camelCaseThis elsewhere?  I think usually unregister rather than unRegister?

> Source/WebCore/storage/IDBDatabaseCallbacksImpl.h:51
> +    // If a RefPtr were used here, there'd be a cycle and the ref counting

It seems like this could be more concise.

[David Grogan]

Created attachment 85859 [details]
Patch

[David Grogan]

Comment on attachment 85827 [details]
Patch

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

>> Source/WebCore/storage/IDBDatabaseCallbacksImpl.h:45
>> +    void unRegisterDatabase(IDBDatabase*);
> 
> How do we camelCaseThis elsewhere?  I think usually unregister rather than unRegister?

You're right, changed.

>> Source/WebCore/storage/IDBDatabaseCallbacksImpl.h:51
>> +    // If a RefPtr were used here, there'd be a cycle and the ref counting
> 
> It seems like this could be more concise.

Shortened.

[Jeremy Orlow]

Comment on attachment 85859 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 85859 [details]
Patch

Clearing flags on attachment: 85859

Committed r81181: <http://trac.webkit.org/changeset/81181>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.