Summary: | Don't allow sudden termination while writing to local storage. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Roger Fong <roger_fong> | ||||||||
Component: | WebKit Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | andersca, beidson, buildbot, bunhere, cdumez, commit-queue, darin, gyuyoung.kim, mitz, rniwa, roger_fong, sergio, thorton | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Roger Fong
2014-06-24 10:10:38 PDT
Created attachment 233717 [details]
patch
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 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
Created attachment 233742 [details]
patch
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? > 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> committed: http://trac.webkit.org/changeset/170471 (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? |