Bug 134254 – Don't allow sudden termination while writing to local storage.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Roger Fong]

<rdar://problem/15093854>

If we terminate the UIProcess while trying to write to local storage we try to finish the write before actually exiting.
However, if the process suddenly terminates then the observer for the application termination notification won't be triggered.
This patch will disable sudden termination while we write to local storage and re-enable when we're done.

[Roger Fong]

Created attachment 233717 [details]
patch

[Build Bot]

Comment on attachment 233717 [details]
patch

Attachment 233717 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4568576023330816

New failing tests:
media/W3C/video/currentSrc/currentSrc_empty_if_no_src.html

[Build Bot]

Created attachment 233737 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Roger Fong]

Created attachment 233742 [details]
patch

[Darin Adler]

Comment on attachment 233742 [details]
patch

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

> Source/WebKit2/ChangeLog:3
> +        Don't allow for sudden termination while writing to local storage.

“Don’t allow sudden termination”

“Don’t allow for” means something different.

> Source/WebKit2/UIProcess/Storage/LocalStorageDatabase.cpp:36
> +#include <WebCore/SuddenTermination.h>

Don’t need to add this both to the header and the cpp file. If you are unable to remove it from the header (see comment below), please remove it from here.

> Source/WebKit2/UIProcess/Storage/LocalStorageDatabase.cpp:269
> +        if (m_disableSuddenTerminationWhileWritingToLocalStorage)
> +            m_disableSuddenTerminationWhileWritingToLocalStorage.reset();

Normally we just assign nullptr instead of using the reset function. Also, it’s overkill to add the if statement. So these two lines should be replaced with just this:

    m_disableSuddenTerminationWhileWritingToLocalStorage = nullptr;

Is there something that guarantees the database is flushed out to disk? How did you test?

> Source/WebKit2/UIProcess/Storage/LocalStorageDatabase.h:30
> +#include <WebCore/SuddenTermination.h>

Do we really need this include? Can we just do a forward declaration instead?

[Roger Fong]

> Is there something that guarantees the database is flushed out to disk? How did you test?

https://bugs.webkit.org/show_bug.cgi?id=134042 guarantees flushing the database out to disk on application termination.
However if application suddenly terminates it is possible that the the application termination handler does not get fired and we don't flush at all.

Tests are provided in <rdar://problem/16660724> and <rdar://problem/15093854>

[Roger Fong]

committed: http://trac.webkit.org/changeset/170471

[Darin Adler]

(In reply to comment #6)
> > Is there something that guarantees the database is flushed out to disk? How did you test?
> 
> https://bugs.webkit.org/show_bug.cgi?id=134042 guarantees flushing the database out to disk on application termination.
> However if application suddenly terminates it is possible that the the application termination handler does not get fired and we don't flush at all.

But what prevents sudden termination between the time that LocalStorageDatabase writes to the database and stops preventing sudden termination, but before the code from bug 134042 is run?