Bug 51112

Summary:

IDBTransactionBackedImpl instances can be accidentally deleted during calls to abort/commit.

Product:

WebKit

Reporter:

Andrei Popescu <andreip>

Component:

WebCore Misc.

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, hans, jorlow

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Attachments:

Description	Flags
Make IDBTransactionBackendImpl objects take a self reference during commit and abort.	eric: review-
test added	jorlow: review+, commit-queue: commit-queue-

Andrei Popescu

Reported 2010-12-15 08:45:12 PST

IDBTransactionBackedImpl instances can be accidentally deleted during calls to abort/commit. This happens because IDBTransactionBackedImpl invokes IDBTransactionCoordinator::didFinishTransaction(), which will release its reference to the IDBTransactionBackedImpl instance. If that was the last reference, the IDBTransactionBackedImpl instance is deleted and we crash.

Attachments
Make IDBTransactionBackendImpl objects take a self reference during commit and abort. (2.07 KB, patch) 2010-12-15 09:01 PST, Andrei Popescu	eric: review-	Details Formatted Diff Diff
test added (5.19 KB, patch) 2010-12-17 11:16 PST, Andrei Popescu	jorlow: review+ commit-queue: commit-queue-	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Andrei Popescu

Comment 1 2010-12-15 09:01:16 PST

Created attachment 76652 [details] Make IDBTransactionBackendImpl objects take a self reference during commit and abort.

Eric Seidel (no email)

Comment 2 2010-12-15 15:24:51 PST

Comment on attachment 76652 [details] Make IDBTransactionBackendImpl objects take a self reference during commit and abort. How do existing tests cover this? Do they crash? If so, which tests are you making not crash? Please explain in your ChangeLog.

Jeremy Orlow

Comment 3 2010-12-16 02:20:25 PST

Besides Eric's comments, it looks good. In theory, it might be possible to repro this in DRT by creating a transaction, leaving the scope where it was created, doing a gc(), and then praying. It wouldn't surprise me if we couldn't come up with a reliable repro, but please give it a shot.

Andrei Popescu

Comment 4 2010-12-16 03:43:57 PST

Andrei Popescu

Comment 5 2010-12-17 11:16:21 PST

Created attachment 76894 [details] test added

Andrei Popescu

Comment 6 2010-12-17 11:18:24 PST

(In reply to comment #3) > Besides Eric's comments, it looks good. In theory, it might be possible to repro this in DRT by creating a transaction, leaving the scope where it was created, doing a gc(), and then praying. It wouldn't surprise me if we couldn't come up with a reliable repro, but please give it a shot. Ok, added a test. I can see in in Visual Studio the deletion happening before accessing member variables of the deleted instance but the test does not always crash for me as the memory isn't always reclaimed. However, if the memory is reclaimed, this test will trigger the crash.

Jeremy Orlow

Comment 7 2010-12-17 11:22:41 PST

Comment on attachment 76894 [details] test added r=me

Jeremy Orlow

Comment 8 2010-12-17 11:24:35 PST

(In reply to comment #6) > (In reply to comment #3) > > Besides Eric's comments, it looks good. In theory, it might be possible to repro this in DRT by creating a transaction, leaving the scope where it was created, doing a gc(), and then praying. It wouldn't surprise me if we couldn't come up with a reliable repro, but please give it a shot. > > Ok, added a test. I can see in in Visual Studio the deletion happening before accessing member variables of the deleted instance but the test does not always crash for me as the memory isn't always reclaimed. However, if the memory is reclaimed, this test will trigger the crash. I'm pretty sure there's nothing much more you can do here. It's definitely better than nothing and tools like Valgrind should catch the errors. Thanks.

WebKit Commit Bot

Comment 9 2010-12-20 06:00:27 PST

The commit-queue encountered the following flaky tests while processing attachment 76894 [details]: fast/preloader/script.html bug 50879 (author: abarth@webkit.org) fast/loader/recursive-before-unload-crash.html bug 50880 (authors: beidson@apple.com and eric@webkit.org) The commit-queue is continuing to process your patch.

WebKit Commit Bot

Comment 10 2010-12-20 06:32:35 PST

Comment on attachment 76894 [details] test added Rejecting attachment 76894 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-3', 'apply-attachment', '--non-interactive', 76894]" exit_code: 2 Last 500 characters of output: /ChangeLog Failed to merge in the changes. Patch failed at 0001 2010-12-20 Yury Semikhatsky <yurys@chromium.org> When you have resolved this problem run "git rebase --continue". If you would prefer to skip this patch, instead run "git rebase --skip". To restore the original branch and stop rebasing run "git rebase --abort". rebase refs/remotes/origin/master: command returned error: 1 Died at Tools/Scripts/update-webkit line 132. Failed to run "['Tools/Scripts/update-webkit']" exit_code: 2 Full output: http://queues.webkit.org/results/7320063

Andrei Popescu

Comment 11 2010-12-20 06:42:46 PST

Committed r74343

Note You need to log in before you can comment on or make changes to this bug.