Bug 69138

Summary:

Local storage getItem() for an empty string returned UNDEFINED value.

Product:

WebKit

Reporter:

Naveen Bobbili <naveenbobbili>

Component:

DOM

Assignee:

Sihui Liu <sihui_liu>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ap, beidson, cdumez, commit-queue, dbates, eric, hi, jberlin, jonathan.dong.webkit, jorlow, michaeln, naveenbobbili, rniwa, sihui_liu, webkit.review.bot

Priority:

Keywords:

InRadar

Version:

528+ (Nightly build)

Hardware:

Other

OS:

Linux

Bug Depends on:

Bug Blocks:

184382

Attachments:

Description	Flags
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.	none
Uploaded patch.	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none

Naveen Bobbili

Reported 2011-09-30 01:18:14 PDT

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.

Attachments
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. (6.54 KB, patch) 2011-09-30 02:41 PDT, Naveen Bobbili	no flags	Details Formatted Diff Diff
Uploaded patch. (6.82 KB, patch) 2011-10-02 22:43 PDT, Naveen Bobbili	no flags	Details Formatted Diff Diff
Patch (5.76 KB, patch) 2018-03-13 13:51 PDT, Sihui Liu	no flags	Details Formatted Diff Diff
Patch (7.34 KB, patch) 2018-03-16 20:26 PDT, Sihui Liu	no flags	Details Formatted Diff Diff
Patch (15.98 KB, patch) 2018-03-17 01:20 PDT, Sihui Liu	no flags	Details Formatted Diff Diff
Patch (15.98 KB, patch) 2018-03-17 01:30 PDT, Sihui Liu	no flags	Details Formatted Diff Diff
Patch (16.78 KB, patch) 2018-03-17 09:17 PDT, Sihui Liu	no flags	Details Formatted Diff Diff
Patch (16.79 KB, patch) 2018-03-17 16:36 PDT, Sihui Liu	no flags	Details Formatted Diff Diff
Patch (16.55 KB, patch) 2018-03-18 05:29 PDT, Sihui Liu	no flags	Details Formatted Diff Diff
Patch (16.47 KB, patch) 2018-03-19 22:06 PDT, Sihui Liu	no flags	Details Formatted Diff Diff
Patch (14.52 KB, patch) 2018-03-23 01:28 PDT, Sihui Liu	no flags	Details Formatted Diff Diff
Patch (14.49 KB, patch) 2018-03-23 14:54 PDT, Sihui Liu	no flags	Details Formatted Diff Diff
Show Obsolete (11) View All Add attachment proposed patch, testcase, etc.

Naveen Bobbili

Comment 1 2011-09-30 02:41:04 PDT

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.

Alexey Proskuryakov

Comment 2 2011-09-30 08:52:39 PDT

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.

Naveen Bobbili

Comment 3 2011-09-30 08:58:27 PDT

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?

Naveen Bobbili

Comment 4 2011-09-30 09:18:54 PDT

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?

Alexey Proskuryakov

Comment 5 2011-09-30 10:22:59 PDT

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.

Naveen Bobbili

Comment 6 2011-09-30 17:42:53 PDT

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.

WebKit Review Bot

Comment 7 2011-10-02 21:47:21 PDT

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.

Alexey Proskuryakov

Comment 8 2011-10-02 22:00:58 PDT

Sorry, this is not my area of expertise, someone else will need to review the patch once you mark it for review.

Naveen Bobbili

Comment 9 2011-10-02 22:43:32 PDT

Created attachment 109438 [details] Uploaded patch. Uploaded Patch

Naveen Bobbili

Comment 10 2011-10-02 22:46:38 PDT

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.

Naveen Bobbili

Comment 11 2011-10-02 23:01:50 PDT

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.

Alexey Proskuryakov

Comment 12 2011-10-03 07:59:17 PDT

> 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.

Jeremy Orlow

Comment 13 2011-10-31 23:57:22 PDT

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?

Ryosuke Niwa

Comment 14 2012-08-07 22:53:17 PDT

Comment on attachment 109438 [details] Uploaded patch. Is this patch still viable?

Michael Nordman

Comment 15 2012-09-13 16:08:59 PDT

Chrome doesn't have this bug, I'm not sure about other ports.

Jessie Berlin

Comment 16 2013-04-01 09:38:00 PDT

(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|"

Jessie Berlin

Comment 17 2013-04-01 09:39:54 PDT

<rdar://problem/13410974>

Anders Carlsson

Comment 18 2014-02-05 10:50:59 PST

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.

Sihui Liu

Comment 19 2018-03-13 13:51:45 PDT

Created attachment 335723 [details] Patch

Chris Dumez

Comment 20 2018-03-13 14:05:59 PDT

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()); ?

Sihui Liu

Comment 21 2018-03-16 20:26:35 PDT

Created attachment 335994 [details] Patch

Sihui Liu

Comment 22 2018-03-17 01:20:07 PDT

Created attachment 336003 [details] Patch

Sihui Liu

Comment 23 2018-03-17 01:30:12 PDT

Created attachment 336004 [details] Patch

Sihui Liu

Comment 24 2018-03-17 09:17:35 PDT

Created attachment 336008 [details] Patch

Chris Dumez

Comment 25 2018-03-17 09:18:27 PDT

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.

Sihui Liu

Comment 26 2018-03-17 16:36:18 PDT

Created attachment 336018 [details] Patch

Sihui Liu

Comment 27 2018-03-18 05:29:38 PDT

Created attachment 336023 [details] Patch

Chris Dumez

Comment 28 2018-03-19 14:26:18 PDT

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.

Ryosuke Niwa

Comment 29 2018-03-19 21:43:48 PDT

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?

Ryosuke Niwa

Comment 30 2018-03-19 21:44:50 PDT

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.

Sihui Liu

Comment 31 2018-03-19 22:06:23 PDT

Created attachment 336102 [details] Patch

Sihui Liu

Comment 32 2018-03-19 22:09:09 PDT

(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.

Ryosuke Niwa

Comment 33 2018-03-19 22:15:49 PDT

(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.

Ryosuke Niwa

Comment 34 2018-03-20 22:12:01 PDT

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.

Sihui Liu

Comment 35 2018-03-21 08:43:53 PDT

(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.

Chris Dumez

Comment 36 2018-03-21 13:35:44 PDT

(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?

Chris Dumez

Comment 37 2018-03-21 13:38:22 PDT

(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.

Ryosuke Niwa

Comment 38 2018-03-21 13:41:51 PDT

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?

Chris Dumez

Comment 39 2018-03-21 13:42:23 PDT

(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.

Chris Dumez

Comment 40 2018-03-21 13:44:01 PDT

(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?

Ryosuke Niwa

Comment 41 2018-03-21 13:49:46 PDT

(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

Sihui Liu

Comment 42 2018-03-21 21:47:32 PDT

(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).

Sihui Liu

Comment 43 2018-03-21 21:48:39 PDT

(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?

Chris Dumez

Comment 44 2018-03-22 08:40:05 PDT

(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.

Sihui Liu

Comment 45 2018-03-22 09:44:41 PDT

(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?

Chris Dumez

Comment 46 2018-03-22 10:12:39 PDT

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(); }

Chris Dumez

Comment 47 2018-03-22 10:50:56 PDT

(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?

Chris Dumez

Comment 48 2018-03-22 10:52:56 PDT

(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().

Sihui Liu

Comment 49 2018-03-23 01:28:45 PDT

Created attachment 336360 [details] Patch

Chris Dumez

Comment 50 2018-03-23 08:51:02 PDT

Comment on attachment 336360 [details] Patch Informal LGTM.

Brady Eidson

Comment 51 2018-03-23 14:10:57 PDT

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()

Chris Dumez

Comment 52 2018-03-23 14:47:44 PDT

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.

Sihui Liu

Comment 53 2018-03-23 14:54:27 PDT

Created attachment 336427 [details] Patch

WebKit Commit Bot

Comment 54 2018-03-23 15:41:45 PDT

Comment on attachment 336427 [details] Patch Clearing flags on attachment: 336427 Committed r229929: <https://trac.webkit.org/changeset/229929>

WebKit Commit Bot

Comment 55 2018-03-23 15:41:47 PDT

All reviewed patches have been landed. Closing bug.

Devin Rousso

Comment 56 2018-04-06 15:53:12 PDT

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.

Devin Rousso

Comment 57 2018-04-06 23:02:56 PDT

(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>.

Note You need to log in before you can comment on or make changes to this bug.