Implement an optional per-site quota for HTML 5 local / session storage

• Component: WebCore Misc.
• Status: NEW ---

[Cameron Zwarich (cpst)]

We should do this.

[Cameron Zwarich (cpst)]

<rdar://problem/6943189>

[Cameron Zwarich (cpst)]

Created attachment 30892 [details]
Proposed patch

[Cameron Zwarich (cpst)]

Created attachment 30893 [details]
Proposed patch (with better ChangeLog)

[Jeremy Orlow]

This patch includes some of the EXACT errors I pointed out from https://bugs.webkit.org/show_bug.cgi?id=22765

For example:
"if (value != oldValue)"
should probably be
"if (!oldValue.isNull())"

And:
"m_totalUsage = m_totalUsage + value.length() * sizeof(UChar) - oldValue.length() * sizeof(UChar);"
should probably just use a +=

And I still don't like the idea of doing the same computations in 2 places (the StorageArea and the StorageMap).

[Jeremy Orlow]

Also, since this is for a fixed quota, it seems like you could easily add a layout test.

[Cameron Zwarich (cpst)]

Created attachment 30894 [details]
Proposed patch (with better ChangeLog)

Does anyone have a better name for the ENABLE macro?

[Cameron Zwarich (cpst)]

Oh, I didn't see Jeremy's comment that there was already a bug with a stale patch for this. I'll take a look at the problems there.

[Cameron Zwarich (cpst)]

(In reply to comment #4)
> This patch includes some of the EXACT errors I pointed out from
> https://bugs.webkit.org/show_bug.cgi?id=22765
> 
> For example:
> "if (value != oldValue)"
> should probably be
> "if (!oldValue.isNull())"

That sounds good.

> And:
> "m_totalUsage = m_totalUsage + value.length() * sizeof(UChar) -
> oldValue.length() * sizeof(UChar);"
> should probably just use a +=

Yeah, I should probably do that. I almost changed it, but I left it that way because I wanted to be more explicit about the weirdness around the mixing of signed/unsigned 32-bit/64-bit types.

> And I still don't like the idea of doing the same computations in 2 places (the
> StorageArea and the StorageMap).

What do you propose? Make it so StorageMap::setItem() can fail due to a quota failure and then deal with it in the caller? That might be okay.

You mention quota management on the other bug. This probably requires a lot more code than what is here. You would need to have separate quotas for local and session storage per origin. For SQL storage, the quotas piggyback on the database, but we would need to add separate ChromeClient methods for getting the quota for each page and type of storage, asking the user to increase it, etc. Is this really necessary for a first cut?

[Cameron Zwarich (cpst)]

Comment on attachment 30894 [details]
Proposed patch (with better ChangeLog)

r-ing this patch to deal with some comments.

[Jeremy Orlow]

(In reply to comment #8)
> (In reply to comment #4)
> > This patch includes some of the EXACT errors I pointed out from
> > https://bugs.webkit.org/show_bug.cgi?id=22765
> > 
> > For example:
> > "if (value != oldValue)"
> > should probably be
> > "if (!oldValue.isNull())"
> 
> That sounds good.
> 
> > And:
> > "m_totalUsage = m_totalUsage + value.length() * sizeof(UChar) -
> > oldValue.length() * sizeof(UChar);"
> > should probably just use a +=
> 
> Yeah, I should probably do that. I almost changed it, but I left it that way
> because I wanted to be more explicit about the weirdness around the mixing of
> signed/unsigned 32-bit/64-bit types.

I see.  I guess that makes sense.  Maybe you should change some of the existing +='s to just blah = blah + ... for consistency?  I know the first time I read that code, I was confused for a moment because of the difference.  Not a big deal though.
 
> > And I still don't like the idea of doing the same computations in 2 places (the
> > StorageArea and the StorageMap).
> 
> What do you propose? Make it so StorageMap::setItem() can fail due to a quota
> failure and then deal with it in the caller? That might be okay.

That is what I'd suggest.  I'd probably just have setItem return a bool that says whether or not it succeeded and have the StorageArea::setItem code check for that and set the exception code.

> You mention quota management on the other bug. This probably requires a lot
> more code than what is here. You would need to have separate quotas for local
> and session storage per origin. For SQL storage, the quotas piggyback on the
> database, but we would need to add separate ChromeClient methods for getting
> the quota for each page and type of storage, asking the user to increase it,
> etc. Is this really necessary for a first cut?

No.  Think this is a good first cut and that a second package can clean it up.  But I'm a bit biased because I'm doing some re-factoring work in the space that would conflict with quota-management like what was in the other patch or even what I suggested.  :-)