Bug 184382 - REGRESSION(r229929): localStorage is broken for WebInspector
Summary: REGRESSION(r229929): localStorage is broken for WebInspector
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P1 Normal
Assignee: Sihui Liu
URL:
Keywords: InRadar, Regression
Depends on: 69138
Blocks:
  Show dependency treegraph
 
Reported: 2018-04-06 23:02 PDT by Devin Rousso
Modified: 2018-04-09 17:17 PDT (History)
9 users (show)

See Also:


Attachments
Patch (1.85 KB, patch)
2018-04-07 20:14 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (1.89 KB, patch)
2018-04-08 01:51 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (4.32 KB, patch)
2018-04-09 10:46 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (4.31 KB, patch)
2018-04-09 10:59 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-sierra (2.28 MB, application/zip)
2018-04-09 12:04 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2018-04-06 23:02:12 PDT
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.
Comment 1 Radar WebKit Bug Importer 2018-04-06 23:04:03 PDT
<rdar://problem/39257355>
Comment 2 Sihui Liu 2018-04-07 20:14:05 PDT
Created attachment 337439 [details]
Patch
Comment 3 Daniel Bates 2018-04-07 23:56:05 PDT
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?
Comment 4 Sihui Liu 2018-04-08 01:51:36 PDT
Created attachment 337453 [details]
Patch
Comment 5 Joseph Pecoraro 2018-04-08 02:28:05 PDT
When is the error ever cleared? If there was an error before entering this method would it have been cleared?
Comment 6 Chris Dumez 2018-04-09 09:07:42 PDT
(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
Comment 7 Chris Dumez 2018-04-09 09:08:38 PDT
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.
Comment 8 Sihui Liu 2018-04-09 10:46:57 PDT
Created attachment 337507 [details]
Patch
Comment 9 Sihui Liu 2018-04-09 10:59:32 PDT
Created attachment 337510 [details]
Patch
Comment 10 Build Bot 2018-04-09 12:04:09 PDT
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
Comment 11 Build Bot 2018-04-09 12:04:11 PDT
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
Comment 12 Chris Dumez 2018-04-09 16:51:25 PDT
Comment on attachment 337510 [details]
Patch

failure is unrelated.
Comment 13 WebKit Commit Bot 2018-04-09 17:17:54 PDT
Comment on attachment 337510 [details]
Patch

Clearing flags on attachment: 337510

Committed r230456: <https://trac.webkit.org/changeset/230456>
Comment 14 WebKit Commit Bot 2018-04-09 17:17:56 PDT
All reviewed patches have been landed.  Closing bug.