Bug 32913 - Do not a new Database pointer to any structure until its version has been verified
Summary: Do not a new Database pointer to any structure until its version has been ver...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Dumitru Daniliuc
URL:
Keywords:
Depends on: 32955
Blocks:
  Show dependency treegraph
 
Reported: 2009-12-23 17:08 PST by Dumitru Daniliuc
Modified: 2009-12-28 18:47 PST (History)
7 users (show)

See Also:


Attachments
patch (2.62 KB, patch)
2009-12-23 17:11 PST, Dumitru Daniliuc
eric: review+
dumi: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dumitru Daniliuc 2009-12-23 17:08:35 PST
We should add the pointer to a new Database to DatabaseTracker, Document, and DatabaseThread only after opening the database and successfully verifying its version.
Comment 1 Dumitru Daniliuc 2009-12-23 17:11:31 PST
Created attachment 45458 [details]
patch
Comment 2 WebKit Review Bot 2009-12-23 17:17:02 PST
style-queue ran check-webkit-style on attachment 45458 [details] without any errors.
Comment 3 Eric Seidel (no email) 2009-12-23 17:19:35 PST
Comment on attachment 45458 [details]
patch

Change looks great!  How do we test this?
Comment 4 Eric Seidel (no email) 2009-12-23 17:27:51 PST
Comment on attachment 45458 [details]
patch

Please update the ChangeLog when you land to point out:

1.  What test this fixes.
2.  What revision caused this regression.
3.  That this is in fact a regression fix.

A common way to do that is for the bug title to have REGRESSION(r12345): in it, and to link to the regrssion commit from the Changelog:
http://trac.webkit.org/changeset/12345

And then just to list the test that it fixes!

I certainly don't need to see this again. :)  Thanks for the quick fix!
Comment 5 Dumitru Daniliuc 2009-12-23 17:39:34 PST
Sorry, didn't know that: REGRESSION(r52530). Updated the ChangeLog file accordingly too.

Landed as r52536.
Comment 6 Shinichiro Hamaji 2009-12-24 19:25:24 PST
Committed r52554: <http://trac.webkit.org/changeset/52554>
Comment 7 Daniel Bates 2009-12-25 15:03:45 PST
This change (r52554) caused layout test failures on the Leopard Intel Debug (Tests) bot.

The following are the failing tests (*):

storage/open-database-while-transaction-in-progress.html -> crashed
svg/W3C-SVG-1.1/filters-conv-01-f.svg -> failed

See <http://build.webkit.org/results/Leopard%20Intel%20Debug%20(Tests)/r52554%20(8651)/> for test output/stderrs.

(*) From <http://build.webkit.org/builders/Leopard%20Intel%20Debug%20%28Tests%29/builds/8651/steps/layout-test/logs/stdio>
Comment 8 Eric Seidel (no email) 2009-12-26 20:49:55 PST
Shinichiro, what broke?  Your rollout change does not mention what broke:
http://trac.webkit.org/changeset/52554

Also please be sure to re-open bugs if you roll them out.

Also, it looks like your rollout broke two tests.  :(
https://bugs.webkit.org/show_bug.cgi?id=32955

I'm sure you didn't mean to do all this, but it seems you were very unlucky this time. :(

I'm not sure how the rollout could have caused the SVG failure, but it started failing after r52554 and has been failing since.
Comment 9 Eric Seidel (no email) 2009-12-26 21:14:10 PST
I'm going to roll-out the rollout for now.  I'll test it locally first to make sure all tests pass on Leopard.
Comment 10 Dumitru Daniliuc 2009-12-28 18:47:47 PST
I figured out the problem. My patch was technically correct (and that's why WebKit was happy with it). However, it changed the ordering of some actions in a subtle way and that broke an assumption on which the Chromium implementation was built.

I opened bug 33005 to track this issue and will soon upload/commit a patch that should make both WebKit and Chromium happy.

Closing this bug.