Bug 40301 – DOM storage should only create databases when needed

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Hans Wennborg]

As soon as a page attempts to use localstorage, StorageAreaSync will create an empty database if one does not already exist. This can lead to lots of unnecessary database files. In particular, they are created even when the privacy settings or private browsing mode disallow localstorage data, which may seem odd to the user.

For example, visit http://people.w3.org/mike/localstorage.html and note that ~/Library/Safari/LocalStorage/http_people.w3.org_0.localstorage is created even though no attempt to store any data has been made.

Database creation should be put off in StorageAreaSync until it is time to actually write something to the database.

[Hans Wennborg]

Created attachment 58140 [details]
Patch

[Jeremy Orlow]

Comment on attachment 58140 [details]
Patch

WebCore/storage/StorageAreaSync.cpp:306
 +      if (!m_database.isOpen())
Is it safe to do this?  What happens if it fails the first 2 times (the HD is full or something) but later starts succeeding.  Please verify it'll do the right thing.

I'd lean towards a m_initialized flag.

But is any of this needed?  Isn't the initial import guaranteed to run first?

WebCore/storage/StorageAreaSync.cpp:202
 +  void StorageAreaSync::openDatabase(bool create)
Create an enum instead of a bool for create and don't create.  It makes code easier to read.

WebCore/ChangeLog:12
 +          Tested by visiting e.g. http://people.w3.org/mike/localstorage.html and verifying that no file is created in ~/Library/Safari/LocalStorage until something is stored.
Create a manual-test in WebCore/manual-tests

[Hans Wennborg]

(In reply to comment #2)
> (From update of attachment 58140 [details])
> WebCore/storage/StorageAreaSync.cpp:306
>  +      if (!m_database.isOpen())
> Is it safe to do this?  What happens if it fails the first 2 times (the HD is full or something) but later starts succeeding.  Please verify it'll do the right thing.
It's not safe in that if it first fails to open the database, and later starts succeeding, it will have dropped some sync items and the database contents might not correctly represent the state of the storage. Changing so that if it fails to open the database once, it will not retry.

> 
> I'd lean towards a m_initialized flag.
> 
> But is any of this needed?  Isn't the initial import guaranteed to run first?
> 
Yes, but the initial import no longer always opens the database; that's why it's needed.

> WebCore/storage/StorageAreaSync.cpp:202
>  +  void StorageAreaSync::openDatabase(bool create)
> Create an enum instead of a bool for create and don't create.  It makes code easier to read.
Done. Suggestions for better naming welcome.

> 
> WebCore/ChangeLog:12
>  +          Tested by visiting e.g. http://people.w3.org/mike/localstorage.html and verifying that no file is created in ~/Library/Safari/LocalStorage until something is stored.
> Create a manual-test in WebCore/manual-tests
Done.

[Hans Wennborg]

Created attachment 58154 [details]
Patch

[Jeremy Orlow]

Comment on attachment 58154 [details]
Patch

WebCore/storage/StorageAreaSync.cpp:66
 +      , m_triedOpeningDatabase(false)
How about m_databaseOpenFailed

WebCore/storage/StorageAreaSync.cpp:206
 +      ASSERT(!m_database.isOpen());
Assert we haven't tried yet.

WebCore/storage/StorageAreaSync.h:79
 +            CreateDatabase,
These names are not very clear.  The point is that creation is allowed, but this sounds like you're saying always create it.

WebCore/storage/StorageAreaSync.cpp:307
 +      if (!m_database.isOpen() && !m_triedOpeningDatabase)
If you change the name as I suggested, then you can simply do "if m_databaseOpenFailed return;" before this statement.  I think it's more clear.

[Hans Wennborg]

(In reply to comment #5)
> (From update of attachment 58154 [details])
> WebCore/storage/StorageAreaSync.cpp:66
>  +      , m_triedOpeningDatabase(false)
> How about m_databaseOpenFailed
Done.

> WebCore/storage/StorageAreaSync.cpp:206
>  +      ASSERT(!m_database.isOpen());
> Assert we haven't tried yet.
Done.

> WebCore/storage/StorageAreaSync.h:79
>  +            CreateDatabase,
> These names are not very clear.  The point is that creation is allowed, but this sounds like you're saying always create it.
Done. Trying new names.

> WebCore/storage/StorageAreaSync.cpp:307
>  +      if (!m_database.isOpen() && !m_triedOpeningDatabase)
> If you change the name as I suggested, then you can simply do "if m_databaseOpenFailed return;" before this statement.  I think it's more clear.
Done.

[Hans Wennborg]

Created attachment 58233 [details]
Patch

[Jeremy Orlow]

Comment on attachment 58233 [details]
Patch

WebCore/ChangeLog:8
 +          As soon as a page attempts to use localstorage, StorageAreaSync will create an empty database if one doesn't already exist. This can lead to lots of unnecessary database files. In particular, they are created even when the privacy settings or private browsing mode disallow localstorage data, which may seem odd to the user.
This is all on one line.  Please wrap.

WebCore/manual-tests/localstorage-empty-database.html:6
 +  </body>
Why end the body before the body?  :-)

WebCore/storage/StorageAreaSync.cpp:203
 +  void StorageAreaSync::openDatabase(OpenDatabaseParamType create)
Create is probably not the best name for this.

[Hans Wennborg]

(In reply to comment #8)
> (From update of attachment 58233 [details])
> WebCore/ChangeLog:8
>  +          As soon as a page attempts to use localstorage, StorageAreaSync will create an empty database if one doesn't already exist. This can lead to lots of unnecessary database files. In particular, they are created even when the privacy settings or private browsing mode disallow localstorage data, which may seem odd to the user.
> This is all on one line.  Please wrap.
Done.

> WebCore/manual-tests/localstorage-empty-database.html:6
>  +  </body>
> Why end the body before the body?  :-)
Fixed.

> WebCore/storage/StorageAreaSync.cpp:203
>  +  void StorageAreaSync::openDatabase(OpenDatabaseParamType create)
> Create is probably not the best name for this.
Trying new name.

[Hans Wennborg]

Created attachment 58463 [details]
Patch

[Jeremy Orlow]

Comment on attachment 58463 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 58463 [details]
Patch

Clearing flags on attachment: 58463

Committed r61023: <http://trac.webkit.org/changeset/61023>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.