Bug 145549 – REGRESSION(r185091): Crash happens on indexdb tests

• Component: New Bugs
• Status: RESOLVED FIXED

[Gyuyoung Kim]

Some indexeddb tests come to crash since r185091.

> Regressions: Unexpected crashes (5)
>   storage/indexeddb/error-causes-abort-by-default.html [ Crash ]
>   storage/indexeddb/exception-in-event-aborts.html [ Crash ]
>   storage/indexeddb/mozilla/add-twice-failure.html [ Crash ]
>   storage/indexeddb/request-event-propagation.html [ Crash ]
>   storage/indexeddb/transaction-event-propagation.html [ Crash ]

[Gyuyoung Kim]

Created attachment 254058 [details]
Patch

[Gyuyoung Kim]

Created attachment 254059 [details]
Patch

[Csaba Osztrogonác]

Comment on attachment 254059 [details]
Patch

rs=me

[WebKit Commit Bot]

Comment on attachment 254059 [details]
Patch

Clearing flags on attachment: 254059

Committed r185107: <http://trac.webkit.org/changeset/185107>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 254059 [details]
Patch

This doesn’t look right. It just looks like a storage leak to me. Could you explain what code calls deref on these errors?

[Darin Adler]

I am almost certain this patch is wrong. It’s possible adding a call to WTF::move() around the create functions will fix things; the trick is to change a temporary Ref into a RefPtr. I don’t really understand why, but I am quite sure that by calling leakRef we are actually leaking the objects, so I know that can’t be right. I don’t fully understand why the original crash was happening, though. Should have worked fine.

[Gyuyoung Kim]

Reopening to attach new patch.

[Gyuyoung Kim]

Created attachment 254244 [details]
Patch

[Gyuyoung Kim]

(In reply to comment #6)
> Comment on attachment 254059 [details]
> Patch
> 
> This doesn’t look right. It just looks like a storage leak to me. Could you
> explain what code calls deref on these errors?

I guessed the deref was called by AsyncRequest::completeRequest() though, now I think I can't be 100% sure about it. 

http://trac.webkit.org/browser/trunk/Source/WebKit2/Shared/AsyncRequest.h#L101

> I am almost certain this patch is wrong. It’s possible adding a call to
> WTF::move() around the create functions will fix things; the trick is to
> change a temporary Ref into a RefPtr. I don’t really understand why, but I
> am quite sure that by calling leakRef we are actually leaking the objects,
> so I know that can’t be right. I don’t fully understand why the original
> crash was happening, though. Should have worked fine.

I agree that my previous fix can cause a storage leak because AsyncRequest::completeRequest() may not handle the pointer of IDBDatabaseError instance. However I would like to fix this problem with correct solution. So I upload a revert patch only for this issue. When I have correct solution for this issue, I will upload it again.

[WebKit Commit Bot]

Comment on attachment 254244 [details]
Patch

Clearing flags on attachment: 254244

Committed r185233: <http://trac.webkit.org/changeset/185233>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.