[webkit-reviews] review denied: [Bug 110557] DatabaseTracker::getMaxSizeForDatabase() can return an erroneously large value : [Attachment 189687] the fix

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Mark Lam
<mark.lam at apple.com>'s request for review:
Bug 110557: DatabaseTracker::getMaxSizeForDatabase() can return an erroneously
large value
https://bugs.webkit.org/show_bug.cgi?id=110557

Attachment 189687: the fix
https://bugs.webkit.org/attachment.cgi?id=189687&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=189687&action=review


Can't you test this?

For example, wouldn't this go to the branch?:
1) Assign a large quota.
2) Create a giant database
3) Reduce the Quota
4) ...
5) Profit!

> Source/WebCore/ChangeLog:10
> +
> +	   No new tests.
> +

Maybe add a word of explanation in the ChangeLog.

Why no test?

> Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:312
> +    // The quota comes from a configuration value while the currentUsage
comes
> +    // from a measurement of the real disk usage. Let's consider a situation

> +    // where there is a bug (regardless of the source) where the
currentUsage gets
> +    // unreasonably large such that currentUsage > quota + databaseSize. In
that
> +    // case, maxSize, computed as (quota - currentUsage + databaseFileSize)
will
> +    // be negative and will be misinterpreted as a very large number,
thereby
> +    // effectively granting the database permission to grow unboundedly
beyond the
> +    // quota.
> +    //
> +    // Hence, we will do some explicit checks here to guard against this,
and
> +    // ensure some sanity.

I am really not a fan of this comment. It reads like you have a mysterious bug
elsewhere, and instead of finding it, you just added this workaround here.
If you add an explanation here, it should be of the real use case, not a bug
(regardless of the source).