Bug 31388 – Fix a ref-counting bug in Database.cpp

• Status: RESOLVED
• Resolution: INVALID
• Component: New Bugs

[Dumitru Daniliuc]

WebCore/storage/Database.cpp has a ref-counting bug that needs to be fixed.

[Dumitru Daniliuc]

Created attachment 43024 [details]
patch

Forgot to mention that dglazkov found the root cause of the problem and came up with a solution.

[David Levin]

Comment on attachment 43024 [details]
patch

r- for no explanation of fix in change log (and it looks equivalent to me, sorry that I don't see it).


> Index: WebCore/ChangeLog
> [phone]  Dumitru Daniliuc  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Database.cpp does not correctly handle ref-counting in
> +	Database::openDatabase().

Please remove the TAB here.

Also please explain the bug that is fixed. The code looks equivalent to me.


> Index: WebCore/storage/Database.cpp
> +PassRefPtr<Database> Database::create(Document* document, const String& name, const String& expectedVersion, const String& displayName, unsigned long estimatedSize) {

The open brace for the function needs to go on the next line.


> Index: WebCore/storage/Database.h
> +    static PassRefPtr<Database> create(Document* document, const String& name, const String& expectedVersion, const String& displayName, unsigned long estimatedSize);

The param name "document" shouldn't be here.

[Dumitru Daniliuc]

Created attachment 43025 [details]
patch

(In reply to comment #2)
> (From update of attachment 43024 [details])
> r- for no explanation of fix in change log (and it looks equivalent to me,
> sorry that I don't see it).
> 
> 
> > Index: WebCore/ChangeLog
> > [phone]  Dumitru Daniliuc  <[e-mail]>
> > +
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        Database.cpp does not correctly handle ref-counting in
> > +	Database::openDatabase().
> 
> Please remove the TAB here.

done.

> Also please explain the bug that is fixed. The code looks equivalent to me.

i'm still not very clear on PassRefPtr usages, so i might be wrong here, but i think:

RefPtr<Foo> foo = adoptRef(new Foo()); // foo has a ref-count of 0

PassRefPtr<Foo> getFoo() { return adoptRef(new Foo()); }
RefPtr<Foo> foo = getFoo(); // foo has a ref-count of 1

(i'll ask dglazkov for more details, but there certainly _is_ a difference between these two (i checked))

> > Index: WebCore/storage/Database.cpp
> > +PassRefPtr<Database> Database::create(Document* document, const String& name, const String& expectedVersion, const String& displayName, unsigned long estimatedSize) {
> 
> The open brace for the function needs to go on the next line.

done. also fixed the indentation to 4 spaces.

> > Index: WebCore/storage/Database.h
> > +    static PassRefPtr<Database> create(Document* document, const String& name, const String& expectedVersion, const String& displayName, unsigned long estimatedSize);
> 
> The param name "document" shouldn't be here.

removed. also removed it from openDatabase().

[David Levin]

Issue 1:The core problem is that no one (Maciej, Mark, and myself) understand why this fixes anything.

If there is some problem in the old code, then there is likely a bug in the underlying methods called because it all looks like correct usage that should result in the correct ref count.

oth, if this is just fixing the code to follow the "create pattern", just say so in the changelog and that is fine.


Issue 2: Maciej pointed out create shouldn't be public.

Thanks!

[Dimitri Glazkov (Google)]

> 
> RefPtr<Foo> foo = adoptRef(new Foo()); // foo has a ref-count of 0
> 
> PassRefPtr<Foo> getFoo() { return adoptRef(new Foo()); }
> RefPtr<Foo> foo = getFoo(); // foo has a ref-count of 1

Are you sure it's not 1 and 2? Let's chat tomorrow to figure this out.

[Dumitru Daniliuc]

Please ignore this patch. I must've been dealing with a "dirty" build, or something else that got resolved as soon as I changed this code. Apologies for wasting your time.