Bug 145549

Summary: REGRESSION(r185091): Crash happens on indexdb tests
Product: WebKit Reporter: Gyuyoung Kim <gyuyoung.kim>
Component: New BugsAssignee: Gyuyoung Kim <gyuyoung.kim>
Status: RESOLVED FIXED    
Severity: Normal CC: alecflett, commit-queue, darin, jsbell, ossy
Priority: P2    
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.