Bug 184382 – REGRESSION(r229929): localStorage is broken for WebInspector

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Devin Rousso]

The following lines added in <https://webkit.org/b/69138> appear to have broken WebInspector's usage of `window.localStorage`:

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

NOTES:
The value of `m_database.lastErrorMessage()` seems to be "unknown error" throughout `getColumnBlobAsString` (this was determined by using `syslog(LOG_ERR, "%s", m_database.lastErrorMsg());`).
In the case that `m_database.lastError() != SQLITE_OK`, `blob` isn't falsy and `getColumnBlobAsString` doesn't early return.

[Radar WebKit Bug Importer]

<rdar://problem/39257355>

[Sihui Liu]

Created attachment 337439 [details]
Patch

[Daniel Bates]

Comment on attachment 337439 [details]
Patch

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

> Source/WebCore/ChangeLog:15
> +        No new tests, removed an insufficient judgement to fix regression. 

Is it not possible to write a test for this?

[Sihui Liu]

Created attachment 337453 [details]
Patch

[Joseph Pecoraro]

When is the error ever cleared? If there was an error before entering this method would it have been cleared?

[Chris Dumez]

(In reply to Joseph Pecoraro from comment #5)
> When is the error ever cleared? If there was an error before entering this
> method would it have been cleared?

From the doc [1]:
"""
If the most recent sqlite3_* API call associated with database connection D failed, then the sqlite3_errcode(D) interface returns the numeric result code or extended result code for that API call. If the most recent API call was successful, then the return value from sqlite3_errcode() is undefined.
"""

So my understanding is:
- If the call to sqlite3_column_blob() failed, sqlite3_errcode() will return a valid error code.
- If the call to sqlite3_column_blob() succeeded, sqlite3_errcode() may return whatever.

Unfortunately, it does not look like there is a good way to check if the call to sqlite3_column_blob() failed. This is because as per [2] sqlite3_column_blob() can return NULL due to:
- An error
- The blob has zero length

[1] https://www.sqlite.org/c3ref/errcode.html
[2] https://www.sqlite.org/c3ref/column_blob.html

[Chris Dumez]

Comment on attachment 337453 [details]
Patch

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

> Source/WebCore/platform/sql/SQLiteStatement.cpp:388
> +        if (!(error == SQLITE_OK || error == SQLITE_ROW || error == SQLITE_DONE))

As per the doc, "If the most recent API call was successful, then the return value from sqlite3_errcode() is undefined." Therefore, I do not think this patch is correct.

[Sihui Liu]

Created attachment 337507 [details]
Patch

[Sihui Liu]

Created attachment 337510 [details]
Patch

[EWS Watchlist]

Comment on attachment 337510 [details]
Patch

Attachment 337510 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/7255578

New failing tests:
imported/w3c/web-platform-tests/workers/name-property.html

[EWS Watchlist]

Created attachment 337518 [details]
Archive of layout-test-results from ews101 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Chris Dumez]

Comment on attachment 337510 [details]
Patch

failure is unrelated.

[WebKit Commit Bot]

Comment on attachment 337510 [details]
Patch

Clearing flags on attachment: 337510

Committed r230456: <https://trac.webkit.org/changeset/230456>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.