[Webkit-unassigned] [Bug 58762] localStorage string values truncated at \x00 characters on browser restart

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=58762


Jeremy Orlow <jorlow at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #99775|review?                     |review-
               Flag|                            |




--- Comment #12 from Jeremy Orlow <jorlow at webkit.org>  2011-07-06 22:02:27 PST ---
(From update of attachment 99775)
View in context: https://bugs.webkit.org/attachment.cgi?id=99775&action=review

looking pretty good

> Source/WebCore/ChangeLog:7
> +

Data migrations scare me and people like release engineers and QA...might be worth a bit more of a note with something like DATA MIGRATION in all caps so it can catch the right people's attention if they're skimming the ChangeLog.  :-)

> Source/WebCore/manual-tests/localstorage-value-truncation.html:18
> +        document.write("</p><p>The expected value is: '123\x0056', the expected length is: 6</p>");

You should also be able to write a big pass or fail for whether it seems correct.

> Source/WebCore/storage/StorageAreaSync.cpp:270
> +    SQLiteStatement query(m_database, "SELECT key, value FROM ItemTable");

This will be very heavyweight.  There must be some way to examine just the column.  If not, at least limit it to one row.

> Source/WebCore/storage/StorageAreaSync.cpp:273
> +    query.finalize();

The destructor finalizes, so maybe a bit cleaner to just wrap the block in {}'s so query goes out of scope at this point.

> Source/WebCore/storage/StorageAreaSync.cpp:284
> +    size_t numOfCommands = sizeof(commands) / sizeof(commands[0]);

Does webkit have any macro for this?  I vaguely remember the answer being no, but just double checking.  Alternately, putting a NULL as the last element and looping until you see it might be slightly cleaner.

> Source/WebCore/storage/StorageAreaSync.cpp:286
> +    SQLiteTransaction transaction(m_database, false);

If you're feeling particularly heroic, creating an enum for read only and read write and replacing the second param for SQLiteTransactions would be awesome.  Prob in another patch.  cc me if you decide to do it.

> Source/WebCore/storage/StorageAreaSync.cpp:289
> +        if (!m_database.executeCommand(commands[i])) {

This worries me some...  If this happens, will localStorage not be able to proceed?  I kind of think in such a case we should just drop all the old data (well, save it to some special table name so that it is at least possible to recover it) and move on...

And there should be a NOTREACHED here since someone with it under debugger would probably like to know if we fail.  :-)

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.