Bug 134254 - Don't allow sudden termination while writing to local storage.
Summary: Don't allow sudden termination while writing to local storage.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-06-24 10:10 PDT by Roger Fong
Modified: 2014-06-26 10:01 PDT (History)
13 users (show)

See Also:


Attachments
patch (3.45 KB, patch)
2014-06-24 10:56 PDT, Roger Fong
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 (728.13 KB, application/zip)
2014-06-24 14:03 PDT, Build Bot
no flags Details
patch (3.56 KB, patch)
2014-06-24 14:48 PDT, Roger Fong
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Roger Fong 2014-06-24 10:10:38 PDT
<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.
Comment 1 Roger Fong 2014-06-24 10:56:39 PDT
Created attachment 233717 [details]
patch
Comment 2 Build Bot 2014-06-24 14:03:41 PDT
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
Comment 3 Build Bot 2014-06-24 14:03:45 PDT
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
Comment 4 Roger Fong 2014-06-24 14:48:03 PDT
Created attachment 233742 [details]
patch
Comment 5 Darin Adler 2014-06-25 20:01:52 PDT
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?
Comment 6 Roger Fong 2014-06-25 20:35:30 PDT
> 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>
Comment 7 Roger Fong 2014-06-25 23:51:01 PDT
committed: http://trac.webkit.org/changeset/170471
Comment 8 Darin Adler 2014-06-26 10:01:40 PDT
(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?