1. Use the following code: <html> <head> <script> console.log(localStorage['test1'] + "|"+localStorage['test2']+"|"); localStorage['test1'] = "hi"; localStorage['test2'] = ""; </script> </head> <body> </body> </html> 2. Load webkit gtk browser and on first visit it reads "undefined|undefined" in the console. Reload and you get "hi||". 3. Restart browser and it prints "hi|undefined". What is the expected result? I would expect it to continue to log "hi||" after browser restart. Analysis: This issue has occurred as a result of fix provided to https://bugs.webkit.org/show_bug.cgi?id=58762 Blob datatype return NULL even for empty strings. So this needs to be specially handled in StorageAreaSync code.
Created attachment 109271 [details] Patch for creating a new column in the local storage to keep of the type of blob (whether normal or empty), DB migration code also added. Manual test case added to test the changes. Patch for creating a new column in the local storage to keep of the type of blob (whether normal or empty), DB migration code also added. Manual test case added to test the changes.
Please add a ChangeLog, and mark the patch for review. We really need an automated way to test such things - no one ever runs manual tests.
Ok sure. BTW the change log is already in the patch which is attached. Do I need to do something else to ensure changelog is added?
Also local storage caches values unline IndexedDB. So my automated test needs to restart the browser. As per Jeremy Orlow's comment#8 in https://bugs.webkit.org/show_bug.cgi?id=58762 , we need changes to LayoutTestController to achieve this. Can you provide some direction or test case that i can refer to to achieve this purpose?
Sorry, for some reason I thought that the ChangeLog present in the patch was for LayoutTests directory. Did you use svn-create-patch or webkit-patch to create the patch? They sort patches to put ChangeLog first, which likely got me confused. Yes, we need changes in LayoutTestController to restart the process, or better a method in window.internals that would force a re-read of localStorage. This doesn't necessarily block this patch from being landed, but improving test coverage would be even more important than fixing a bug.
I did an svn-create-patch. I am not sure why its not sorted. I will take some time and figure out how we can improve the test coverage. In the mean while it would be great if you can provide your comments on StorageAreaSync.cpp file. Thanks for your prompt replies.
Attachment 109271 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/manual-tests/localstorage-empty-value.html:1: One or more unexpected \r (^M) found; better to use only a \n [whitespace/carriage_return] [1] Suppressing further [whitespace/carriage_return] reports for this file. Total errors found: 32 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
Sorry, this is not my area of expertise, someone else will need to review the patch once you mark it for review.
Created attachment 109438 [details] Uploaded patch. Uploaded Patch
I believe adding a method in windows.internals would help as the local storage data gets added to DB based on async timer jobs. So by the time we re read forcefully its not guaranteed that data would be available in DB.
Correcting my previous comments. I believe adding a method in windows.internals wouldn't help as the local storage data gets added to DB based on async timer jobs. So by the time we re read forcefully its not guaranteed that data would be available in DB.
> I believe adding a method in windows.internals wouldn't help as the local storage data gets added to DB based on async timer jobs. So by the time we re read forcefully its not guaranteed that data would be available in DB. window.internals can have a flag or callback for ready state if needed.
Comment on attachment 109438 [details] Uploaded patch. View in context: https://bugs.webkit.org/attachment.cgi?id=109438&action=review I know this patch has been here for a while, but honestly I don't feel very comfortable r+ing dom storage patches at this point. There have been other people working in this code since me. Hopefully one of them can take a look? > Source/WebCore/storage/StorageAreaSync.cpp:57 > +#define BLOB_NORMAL (1 << 0) Could some sort of normal null be used instead?
Comment on attachment 109438 [details] Uploaded patch. Is this patch still viable?
Chrome doesn't have this bug, I'm not sure about other ports.
(In reply to comment #15) > Chrome doesn't have this bug, I'm not sure about other ports. When Safari is restarted, it prints "hi|null|"
<rdar://problem/13410974>
Comment on attachment 109438 [details] Uploaded patch. Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.
Created attachment 335723 [details] Patch
Comment on attachment 335723 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=335723&action=review > ChangeLog:9 > + * ManualTests/localstorage-empty-string-value.html: Added. Outdated changelog. Also, this probably needs an API test. > Source/WebKit/UIProcess/WebStorage/LocalStorageDatabase.cpp:47 > +#define BLOB_NORMAL 0 We usually avoid #define statements in WebKit and prefer global statics. e.g. const int normalBlob = 0; const int emptyBlob = 1; However, in this case, I think we don't even need these constants. > Source/WebKit/UIProcess/WebStorage/LocalStorageDatabase.cpp:115 > + if (!m_database.executeCommand("CREATE TABLE IF NOT EXISTS ItemTable (key TEXT UNIQUE ON CONFLICT REPLACE, value BLOB NOT NULL ON CONFLICT FAIL, type INTEGER NOT NULL ON CONFLICT FAIL DEFAULT 0)")) { Maybe rename type to isBlobEmpty ? > Source/WebKit/UIProcess/WebStorage/LocalStorageDatabase.cpp:191 > + int type = query.getColumnInt(2); bool isBlobEmpty = !!query.getColumnInt(2); ? > Source/WebKit/UIProcess/WebStorage/LocalStorageDatabase.cpp:342 > + // If the blob is an empty string, set the type to BLOB_EMPTY. statement.bindInt(3, it->value.isEmpty()); ?
Created attachment 335994 [details] Patch
Created attachment 336003 [details] Patch
Created attachment 336004 [details] Patch
Created attachment 336008 [details] Patch
Comment on attachment 336004 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=336004&action=review I will let somebody else do the formal review given that I helped. > Tools/TestWebKitAPI/Tests/WebKitCocoa/LocalStorageEmptySring.mm:2 > + * Copyright (C) 2016 Apple Inc. All rights reserved. 2018 is the year :) > Tools/TestWebKitAPI/Tests/WebKitCocoa/LocalStorageEmptySring.mm:74 > + // Ditch this web view (ditching its web process) WebKit comments should end with a period. Also what’s important here is not the new WebProcess but the new processpool/datastore. > Tools/TestWebKitAPI/Tests/WebKitCocoa/LocalStorageEmptySring.mm:84 > + // Make a new web view to finish the test Period at the end.
Created attachment 336018 [details] Patch
Created attachment 336023 [details] Patch
Comment on attachment 336023 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=336023&action=review > Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:2157 > + 8CDA4206205CD99400648A15 /* LocalStorageEmptySring.mm */, hmm. there is a typo in your file name :) Also, I would have called this file LocalStoragePersistence.mm so that it is more reusable.
Comment on attachment 336023 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=336023&action=review > Source/WebKit/ChangeLog:3 > + Local storage getItem() for an empty string returned UNDEFINED value. "undefined" should be in lower case since that's the name of the global variable. > Source/WebKit/UIProcess/WebStorage/LocalStorageDatabase.cpp:187 > String value = query.getColumnBlobAsString(1); Why is query.getColumnBlobAsString returning a null string in the case the string was empty?
Our string class (defined in Source/WTF/text/WTFString.h) differentiates empty string and null string so it's odd that we're getting null string (otherwise isNull() would be false) out of sqlite when we stored an empty string.
Created attachment 336102 [details] Patch
(In reply to Ryosuke Niwa from comment #30) > Our string class (defined in Source/WTF/text/WTFString.h) differentiates > empty string and null string so it's odd that we're getting null string > (otherwise isNull() would be false) out of sqlite when we stored an empty > string. I think the problem is Empty BLOB is restored as null string.
(In reply to Sihui Liu from comment #32) > (In reply to Ryosuke Niwa from comment #30) > > Our string class (defined in Source/WTF/text/WTFString.h) differentiates > > empty string and null string so it's odd that we're getting null string > > (otherwise isNull() would be false) out of sqlite when we stored an empty > > string. > > I think the problem is Empty BLOB is restored as null string. If that's the case, can't we simply change the schema to make "value" nullable blob type, and then use NULL in SQL database as null string, and the empty string as empty string? That would be a better reflection of what we're storing.
I talked with Sihui today but I still don't understand why query.getColumnBlobAsString can't simply return an empty string when the blob value is of length 0.
(In reply to Ryosuke Niwa from comment #34) > I talked with Sihui today but I still don't understand why > query.getColumnBlobAsString can't simply return an empty string when the > blob value is of length 0. https://www.sqlite.org/c3ref/column_blob.html As the last 4 paragraphs state, one explanation is it is safer. Put sqlite3_column_blob before sqlite3_column_bytes(get correct content before get size of content) may help avoid type conversion in getColumnBlobAsString. And since sqlite3_column_xx returns 0 or null pointer on memory error, current solution is easier for us to distinguish between empty string and error.
(In reply to Ryosuke Niwa from comment #34) > I talked with Sihui today but I still don't understand why > query.getColumnBlobAsString can't simply return an empty string when the > blob value is of length 0. Per doc: "The return value from sqlite3_column_blob() for a zero-length BLOB is a NULL pointer.". So basically the issue is that it may not be possible to distinquish NULL for an empty BLOB when calling sqlite3_column_blob(). Right?
(In reply to Chris Dumez from comment #36) > (In reply to Ryosuke Niwa from comment #34) > > I talked with Sihui today but I still don't understand why > > query.getColumnBlobAsString can't simply return an empty string when the > > blob value is of length 0. > > Per doc: "The return value from sqlite3_column_blob() for a zero-length BLOB > is a NULL pointer.". So basically the issue is that it may not be possible > to distinquish NULL for an empty BLOB when calling sqlite3_column_blob(). > Right? sqlite3_column_bytes() seems to return 0 for both cases as well.
How about sqlite3_column_count? Can't we discern the error case by checking that sqlite3_column_count is zero (zero: error, non-zero: got results), and return an empty string when sqlite3_column_blob is nullptr?
(In reply to Chris Dumez from comment #37) > (In reply to Chris Dumez from comment #36) > > (In reply to Ryosuke Niwa from comment #34) > > > I talked with Sihui today but I still don't understand why > > > query.getColumnBlobAsString can't simply return an empty string when the > > > blob value is of length 0. > > > > Per doc: "The return value from sqlite3_column_blob() for a zero-length BLOB > > is a NULL pointer.". So basically the issue is that it may not be possible > > to distinquish NULL for an empty BLOB when calling sqlite3_column_blob(). > > Right? > > sqlite3_column_bytes() seems to return 0 for both cases as well. Looking at the doc though, we *may* be able to rely on sqlite3_column_type() to distinguish SQLITE_BLOB, or SQLITE_NULL. If so, we could tweak getColumnBlobAsString() to return a null string if the value is NULL and an empty String ("") if the column is an empty blob.
(In reply to Ryosuke Niwa from comment #38) > How about sqlite3_column_count? Can't we discern the error case by checking > that sqlite3_column_count is zero (zero: error, non-zero: got results), and > return an empty string when sqlite3_column_blob is nullptr? Now I am not sure we're talking about the same thing anymore. Wouldn't sqlite3_column_count() always return the same number of column so matter is the blob column is null or an actual blob?
(In reply to Chris Dumez from comment #40) > (In reply to Ryosuke Niwa from comment #38) > > How about sqlite3_column_count? Can't we discern the error case by checking > > that sqlite3_column_count is zero (zero: error, non-zero: got results), and > > return an empty string when sqlite3_column_blob is nullptr? > > Now I am not sure we're talking about the same thing anymore. Wouldn't > sqlite3_column_count() always return the same number of column so matter is > the blob column is null or an actual blob? yes but it returns 0 when the statement returned no results. See https://www.sqlite.org/c3ref/column_count.html
(In reply to Ryosuke Niwa from comment #41) > (In reply to Chris Dumez from comment #40) > > (In reply to Ryosuke Niwa from comment #38) > > > How about sqlite3_column_count? Can't we discern the error case by checking > > > that sqlite3_column_count is zero (zero: error, non-zero: got results), and > > > return an empty string when sqlite3_column_blob is nullptr? > > > > Now I am not sure we're talking about the same thing anymore. Wouldn't > > sqlite3_column_count() always return the same number of column so matter is > > the blob column is null or an actual blob? > > yes but it returns 0 when the statement returned no results. See > https://www.sqlite.org/c3ref/column_count.html I think you mean sqlite3_data_count(https://www.sqlite.org/c3ref/data_count.html, which may be same as sqlite3_column_count but this states "each row"). sqlite3_data_count would return non-zero when sqlite3_step returns SQLITE_ROW(and this condition is checked before getColumnBlobAsString).
(In reply to Chris Dumez from comment #39) > (In reply to Chris Dumez from comment #37) > > (In reply to Chris Dumez from comment #36) > > > (In reply to Ryosuke Niwa from comment #34) > > > > I talked with Sihui today but I still don't understand why > > > > query.getColumnBlobAsString can't simply return an empty string when the > > > > blob value is of length 0. > > > > > > Per doc: "The return value from sqlite3_column_blob() for a zero-length BLOB > > > is a NULL pointer.". So basically the issue is that it may not be possible > > > to distinquish NULL for an empty BLOB when calling sqlite3_column_blob(). > > > Right? > > > > sqlite3_column_bytes() seems to return 0 for both cases as well. > > Looking at the doc though, we *may* be able to rely on sqlite3_column_type() > to distinguish SQLITE_BLOB, or SQLITE_NULL. > > If so, we could tweak getColumnBlobAsString() to return a null string if the > value is NULL and an empty String ("") if the column is an empty blob. I think the type will not be SQLITE_NULL as the BLOB column is set as NOT NULL?
(In reply to Sihui Liu from comment #43) > (In reply to Chris Dumez from comment #39) > > (In reply to Chris Dumez from comment #37) > > > (In reply to Chris Dumez from comment #36) > > > > (In reply to Ryosuke Niwa from comment #34) > > > > > I talked with Sihui today but I still don't understand why > > > > > query.getColumnBlobAsString can't simply return an empty string when the > > > > > blob value is of length 0. > > > > > > > > Per doc: "The return value from sqlite3_column_blob() for a zero-length BLOB > > > > is a NULL pointer.". So basically the issue is that it may not be possible > > > > to distinquish NULL for an empty BLOB when calling sqlite3_column_blob(). > > > > Right? > > > > > > sqlite3_column_bytes() seems to return 0 for both cases as well. > > > > Looking at the doc though, we *may* be able to rely on sqlite3_column_type() > > to distinguish SQLITE_BLOB, or SQLITE_NULL. > > > > If so, we could tweak getColumnBlobAsString() to return a null string if the > > value is NULL and an empty String ("") if the column is an empty blob. > > I think the type will not be SQLITE_NULL as the BLOB column is set as NOT > NULL? Well, I believe Ryosuke and I are suggesting the mark the column as NULLABLE instead of having an extra column.
(In reply to Chris Dumez from comment #44) > (In reply to Sihui Liu from comment #43) > > (In reply to Chris Dumez from comment #39) > > > (In reply to Chris Dumez from comment #37) > > > > (In reply to Chris Dumez from comment #36) > > > > > (In reply to Ryosuke Niwa from comment #34) > > > > > > I talked with Sihui today but I still don't understand why > > > > > > query.getColumnBlobAsString can't simply return an empty string when the > > > > > > blob value is of length 0. > > > > > > > > > > Per doc: "The return value from sqlite3_column_blob() for a zero-length BLOB > > > > > is a NULL pointer.". So basically the issue is that it may not be possible > > > > > to distinquish NULL for an empty BLOB when calling sqlite3_column_blob(). > > > > > Right? > > > > > > > > sqlite3_column_bytes() seems to return 0 for both cases as well. > > > > > > Looking at the doc though, we *may* be able to rely on sqlite3_column_type() > > > to distinguish SQLITE_BLOB, or SQLITE_NULL. > > > > > > If so, we could tweak getColumnBlobAsString() to return a null string if the > > > value is NULL and an empty String ("") if the column is an empty blob. > > > > I think the type will not be SQLITE_NULL as the BLOB column is set as NOT > > NULL? > > Well, I believe Ryosuke and I are suggesting the mark the column as NULLABLE > instead of having an extra column. If getItems returns null, it means the key doesn't exist (https://www.w3.org/TR/webstorage/#the-storage-interface). So I guess the column's not supposed to accept null value. So are you suggesting making the column nullable and mark empty string as NULL?
I am not familiar with this code / API so this may be silly but why cannot we do this? --- a/Source/WebKit/UIProcess/WebStorage/LocalStorageDatabase.cpp +++ b/Source/WebKit/UIProcess/WebStorage/LocalStorageDatabase.cpp @@ -183,8 +183,8 @@ void LocalStorageDatabase::importItems(StorageMap& storageMap) while (result == SQLITE_ROW) { String key = query.getColumnText(0); String value = query.getColumnBlobAsString(1); - if (!key.isNull() && !value.isNull()) - items.set(key, value); + if (!key.isNull()) + items.set(key, value.isNull() ? emptyString() : value); result = query.step(); }
(In reply to Chris Dumez from comment #46) > I am not familiar with this code / API so this may be silly but why cannot > we do this? > > --- a/Source/WebKit/UIProcess/WebStorage/LocalStorageDatabase.cpp > +++ b/Source/WebKit/UIProcess/WebStorage/LocalStorageDatabase.cpp > @@ -183,8 +183,8 @@ void LocalStorageDatabase::importItems(StorageMap& > storageMap) > while (result == SQLITE_ROW) { > String key = query.getColumnText(0); > String value = query.getColumnBlobAsString(1); > - if (!key.isNull() && !value.isNull()) > - items.set(key, value); > + if (!key.isNull()) > + items.set(key, value.isNull() ? emptyString() : value); > result = query.step(); > } Sihui mentioned offline that there is an issue distinguishing the value being null because the Blob is empty or because a memory allocation error occurred (sqlite3_column_blob() return null either way). I am personally not convinced we need to worry about distinguishing this error case. Ryosuke/Brady, what do you think?
(In reply to Chris Dumez from comment #47) > (In reply to Chris Dumez from comment #46) > > I am not familiar with this code / API so this may be silly but why cannot > > we do this? > > > > --- a/Source/WebKit/UIProcess/WebStorage/LocalStorageDatabase.cpp > > +++ b/Source/WebKit/UIProcess/WebStorage/LocalStorageDatabase.cpp > > @@ -183,8 +183,8 @@ void LocalStorageDatabase::importItems(StorageMap& > > storageMap) > > while (result == SQLITE_ROW) { > > String key = query.getColumnText(0); > > String value = query.getColumnBlobAsString(1); > > - if (!key.isNull() && !value.isNull()) > > - items.set(key, value); > > + if (!key.isNull()) > > + items.set(key, value.isNull() ? emptyString() : value); > > result = query.step(); > > } > > Sihui mentioned offline that there is an issue distinguishing the value > being null because the Blob is empty or because a memory allocation error > occurred (sqlite3_column_blob() return null either way). > > I am personally not convinced we need to worry about distinguishing this > error case. Ryosuke/Brady, what do you think? We we really care about distinguishing this error case, then I think we should update query.getColumnBlobAsString() to return the null String only in case of error, and return the empty String is the Blob is empty. We can know if an error occurred using sqlite3_errcode().
Created attachment 336360 [details] Patch
Comment on attachment 336360 [details] Patch Informal LGTM.
Comment on attachment 336360 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=336360&action=review > Source/WebCore/platform/sql/SQLiteStatement.cpp:386 > + if (sqlite3_errcode(m_database.sqlite3Handle()) != SQLITE_OK) Instead of calling sqlite3_errcode on the sqlite3 handle, just call m_database.lastError()
Comment on attachment 336360 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=336360&action=review > Source/WebCore/ChangeLog:7 > + Reviewed by NOBODY (OOPS!). before you re-upload, update this line to be: Reviewed by Brady Eidson. > Tools/ChangeLog:7 > + Reviewed by NOBODY (OOPS!). before you re-upload, update this line to be: Reviewed by Brady Eidson.
Created attachment 336427 [details] Patch
Comment on attachment 336427 [details] Patch Clearing flags on attachment: 336427 Committed r229929: <https://trac.webkit.org/changeset/229929>
All reviewed patches have been landed. Closing bug.
This patch appears to have broken WebInspector's usage of `window.localStorage`, specifically the following lines: String SQLiteStatement::getColumnBlobAsString(int col) ...| 386| if (m_database.lastError() != SQLITE_OK) 387| return String(); ...| STEPS TO REPRODUCE: 1. Open WebInspector 2. Do anything that would be "saved" for the next load (e.g. move around tabs, change settings, modify sidebar sizes, toggle paint flashing, etc.) 3. Close WebInspector 4. Reopen WebInspector => Any changes previously made will no longer be there (e.g. tabs will be back in original order, settings will revert to default value, all sidebars will be back to default size, paint flashing is disabled, etc.) I've been trying to see if I can print `m_database.lastErrorMsg()`, but am running into linker errors. I'll add another comment if I can figure it out.
(In reply to Devin Rousso from comment #56) > This patch appears to have broken WebInspector's usage of > `window.localStorage`, specifically the following lines: > > String SQLiteStatement::getColumnBlobAsString(int col) > ...| > 386| if (m_database.lastError() != SQLITE_OK) > 387| return String(); > ...| > > STEPS TO REPRODUCE: > 1. Open WebInspector > 2. Do anything that would be "saved" for the next load (e.g. move around > tabs, change settings, modify sidebar sizes, toggle paint flashing, etc.) > 3. Close WebInspector > 4. Reopen WebInspector > => Any changes previously made will no longer be there (e.g. tabs will be > back in original order, settings will revert to default value, all sidebars > will be back to default size, paint flashing is disabled, etc.) > > I've been trying to see if I can print `m_database.lastErrorMsg()`, but am > running into linker errors. I'll add another comment if I can figure it out. Moved to a separate bug <https://webkit.org/b/184382>.