Memory leak using web SQL DB

• Component: WebCore Misc.
• Status: NEW ---

[Michael Nordman]

Here's a repro case...

<script>
	setInterval (function() {
		openDatabase('leaky', '', 'Leaky', 5*1024*1024);
	}, 250);
</script>

There are two references held on Database objects that prevent them from being cleaned up prior to page close even if there are no references or pending completions in the containing script execution content.

1) The background DatabaseThread class holds a ref to each Database instance that's been open on that thread until that Database has been closed. Trouble is, they're not closed until Document shutdown time.

2) The InspectorInstrumentation holds a ref to each Database instance that's been open in InspecotorDatabaseAgent. There is no method to clear the ref for an individual Database, only a way to drop such refs for all Database instance that have been opened in the page. That clearing happens when a new Document is being committed to the Frame.

Also see http://code.google.com/p/chromium/issues/detail?id=62275

[Dan Carney]

Created attachment 159997 [details]
Proposed patch

The above patch stops the leaking of database references in DatabaseThread.  There is still one reference per database name open in the Inspector.

[Build Bot]

Comment on attachment 159997 [details]
Proposed patch

Attachment 159997 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13565396

[David Levin]

Comment on attachment 159997 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=159997&action=review

Quick comments. I didn't verify all the logic. I'll leave that for someone else.

> Source/WebCore/Modules/webdatabase/Database.cpp:71
> +    AtomicallyInitializedStatic(Mutex&, mutex = *new Mutex);

This basically takes a mutex. It is kind of funny if you think about it to take a mutex to create one (and everytime you enter this function as well).

Instead you could use this macro to initialize currentId and then do an atomic increment.

> Source/WebCore/Modules/webdatabase/Database.cpp:210
> +    guard(PassRefPtr<Database>(this), parameters);

This isn't correct "PassRefPtr<Database>(this)". I believe you should either just be able to pass in "this" or there is another construct you can use.

> Source/WebCore/Modules/webdatabase/Database.cpp:276
> +class CloseImmediatelyTask: public ScriptExecutionContext::Task {

space before :

> Source/WebCore/Modules/webdatabase/Database.cpp:279
> +    virtual ~CloseImmediatelyTask() { }

Why?

> Source/WebCore/Modules/webdatabase/Database.cpp:297
> +        m_scriptExecutionContext->postTask(CloseImmediatelyTask::create(this));

Consider using createCallbackTask instead.

[Dan Carney]

Comment on attachment 159997 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=159997&action=review

>> Source/WebCore/Modules/webdatabase/Database.cpp:71
>> +    AtomicallyInitializedStatic(Mutex&, mutex = *new Mutex);
> 
> This basically takes a mutex. It is kind of funny if you think about it to take a mutex to create one (and everytime you enter this function as well).
> 
> Instead you could use this macro to initialize currentId and then do an atomic increment.

I copied the code from elsewhere.  I've updated to use atomicIncrement

>> Source/WebCore/Modules/webdatabase/Database.cpp:276
>> +class CloseImmediatelyTask: public ScriptExecutionContext::Task {
> 
> space before :

done.

>> Source/WebCore/Modules/webdatabase/Database.cpp:279
>> +    virtual ~CloseImmediatelyTask() { }
> 
> Why?

leftovers.  removed

>> Source/WebCore/Modules/webdatabase/Database.cpp:297
>> +        m_scriptExecutionContext->postTask(CloseImmediatelyTask::create(this));
> 
> Consider using createCallbackTask instead.

Unfortunately that doesn't work with PassRefPtr<Database>

[Dan Carney]

Created attachment 160112 [details]
Patch

Comment on attachment 160112 [details]
Patch

Can you write a layout test for this? If you open databases in an endless loop, does this allocate enough memory w/o the patch to crash DRT?


View in context: https://bugs.webkit.org/attachment.cgi?id=160112&action=review

> Source/WebCore/Modules/webdatabase/DatabaseThread.cpp:-118
> -    if (m_openDatabaseSet.size() > 0) {

it seems that this set is only needed to abort open transactions. So what about instead of recording open and close events of the database (via recordDatabaseOpened/Closed), record start and end of transactions? I think it's reasonable to keep a strong reference during transactions. I think that way we might be able to avoid the weak ptr thing all together

[Dan Carney]

Created attachment 169122 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Dan Carney]

(In reply to comment #7)
> Created an attachment (id=169122) [details]
> Patch

Here's a patch which avoids messing with deletion semantics, which is much nicer, but it's a much bigger change.  I can break it down into smaller bits if this is an acceptable way to go.

[Adam Barth]

It's suspicious to me that this code so complicated and that it doesn't use the normal tools we have to manage these sorts of lifetimes (e.g., ActiveDOMObject).  This is old and was written before we really fleshed out those tools.  If possible, I'd prefer to see a patch that simplified this code to use those tools rather than a patch that layered more complexity onto webdatabase's custom system.

[Dan Carney]

(In reply to comment #10)
> It's suspicious to me that this code so complicated and that it doesn't use the normal tools we have to manage these sorts of lifetimes (e.g., ActiveDOMObject).  This is old and was written before we really fleshed out those tools.  If possible, I'd prefer to see a patch that simplified this code to use those tools rather than a patch that layered more complexity onto webdatabase's custom system.

Okay. I'm definitely looking for a better way to do this.  A good portion of this patch is just trying to ensure a particular ScriptExecutionContext lives long enough and is destroyed in the right thread.

[Adam Barth]

Comment on attachment 169122 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=169122&action=review

> Source/WebCore/Modules/webdatabase/Database.cpp:170
> +        ScriptExecutionContext* copy = scriptExecutionContext.get();
> +        copy->postTask(DerefContextTask::create(scriptExecutionContext));

I probably would have picked another name than "copy" here.  It's not really a copy of the scriptExecutionContext (which is non-copiable).

> Source/WebCore/Modules/webdatabase/DatabaseThread.cpp:168
> +    virtual const char* debugTaskName() const { return "CloseDatabaseCloserTask"; }

I presume this is an OVERRIDE

> Source/WebCore/Modules/webdatabase/DatabaseThread.cpp:174
> +    : DatabaseTask(0, 0)
> +    , m_closer(closer)

Bad indent.

> Source/WebCore/Modules/webdatabase/DatabaseThread.cpp:185
> +        m_queue.prepend(CloseDatabaseCloserTask::create(closer));

prepend, interesting.

> Source/WebCore/Modules/webdatabase/DatabaseThread.h:88
> -    typedef HashSet<RefPtr<Database> > DatabaseSet;
> +    typedef HashSet<RefPtr<DatabaseCloser> > DatabaseSet;

I see, this is the point of the whole thing.

> Source/WebKit/chromium/src/WebDatabase.cpp:65
> +    if (!m_stringIdentifier.isNull())
> +        return m_stringIdentifier;

This seems like a hack.  We should just change the API properly.

[Adam Barth]

Dan, I'm sorry, but this patch is too complicated for me to review sensibly.  We need to find folks who actually understand this code to review your patch.  I'm not sure who is the most knowledgable about this code at this point.  Perhaps beidson?

[Dan Carney]

(In reply to comment #13)
> Dan, I'm sorry, but this patch is too complicated for me to review sensibly.  We need to find folks who actually understand this code to review your patch.  I'm not sure who is the most knowledgable about this code at this point.  Perhaps beidson?

Totally understandable. It took me several days to sort out all the various problems that went into this patch, and at the end, this is really just a long serious of hacks strung together because of weird lifecycle and threading issues in the Database code. If there is someone who could review it for me, I'd be happy to write up a quick  summary of the purpose of each hack, so the patch might be more readable.

Comment on attachment 169122 [details]
Patch

I think the change is overall complex enough to warrant a more detailed description in the changelog

View in context: https://bugs.webkit.org/attachment.cgi?id=169122&action=review

> Source/WebCore/Modules/webdatabase/AbstractDatabase.h:60
> +        ~Core();

should be virtual.

[Dan Carney]

Created attachment 171448 [details]
Patch

[Dan Carney]

(In reply to comment #16)
> Created an attachment (id=171448) [details]
> Patch

Addresses Adam's few comments and added a more detailed changelog entry.

[Levi Weintraub]

Should this be moved to blink? If so, please clear the flags on the patch and mark this bug as invalid.

[Alexey Proskuryakov]

This issue doesn't seem to be chromium specific.

[Michael Nordman]

(In reply to comment #19)
> This issue doesn't seem to be chromium specific.

Nope, it is not chromium specific (afaict).

[Anders Carlsson]

Comment on attachment 171448 [details]
Patch

Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.