Bug 145549

Summary:

REGRESSION(r185091): Crash happens on indexdb tests

Product:

WebKit

Reporter:

Gyuyoung Kim <gyuyoung.kim>

Component:

New Bugs

Assignee:

Gyuyoung Kim <gyuyoung.kim>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

alecflett, commit-queue, darin, jsbell, ossy

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

145508

Bug Blocks:

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none

Description Gyuyoung Kim 2015-06-01 22:10:14 PDT

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 ]

Comment 1 Gyuyoung Kim 2015-06-01 23:35:54 PDT

Created attachment 254058 [details]
Patch

Comment 2 Gyuyoung Kim 2015-06-01 23:42:34 PDT

Created attachment 254059 [details]
Patch

Comment 3 Csaba Osztrogonác 2015-06-02 01:23:37 PDT

Comment on attachment 254059 [details]
Patch

rs=me

Comment 4 WebKit Commit Bot 2015-06-02 02:13:05 PDT

Comment on attachment 254059 [details]
Patch

Clearing flags on attachment: 254059

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

Comment 5 WebKit Commit Bot 2015-06-02 02:13:10 PDT

All reviewed patches have been landed.  Closing bug.

Comment 6 Darin Adler 2015-06-02 11:22:49 PDT

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?

Comment 7 Darin Adler 2015-06-02 11:32:14 PDT

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.

Comment 8 Gyuyoung Kim 2015-06-03 21:50:02 PDT

Reopening to attach new patch.

Comment 9 Gyuyoung Kim 2015-06-03 21:50:06 PDT

Created attachment 254244 [details]
Patch

Comment 10 Gyuyoung Kim 2015-06-03 21:59:10 PDT

(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.

Comment 11 WebKit Commit Bot 2015-06-04 18:32:48 PDT

Comment on attachment 254244 [details]
Patch

Clearing flags on attachment: 254244

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

Comment 12 WebKit Commit Bot 2015-06-04 18:32:54 PDT

All reviewed patches have been landed.  Closing bug.