[GTK] LayoutTests/http/tests/appcache/max-size.html fails

• Component: WebKitGTK
• Status: RESOLVED INVALID

[Martin Robinson]

WebKit does not appear to be respecting the application cache size.

[Martin Robinson]

From WebKit/gtk/webkit/webkitapplicationcache.cpp:
    WebCore::cacheStorage().empty();
    WebCore::cacheStorage().vacuumDatabaseFile();
    WebCore::cacheStorage().setMaximumSize(size);

Documentation at http://www.sqlite.org/pragma.html seems to indicate that the vacuum should happen after setting the size though.

[Martin Robinson]

Created attachment 34254 [details]
Vacuum after pragma

Patch which modifies the order of operations in the GTK+ port. If this is a true bug, other ports will likely need to be modified.

[Martin Robinson]

For reference: this was originally failing for me on an Ubuntu Jaunty machine (sqlite 3.6.10-1ubuntu0.2) on x86_64.

I can confirm that this test passes on my Mac both with the vacuum before and after the pragma call. It's probably best to follow the documentation in this case. I'm expanding the patch to include the mac port as well.

[Martin Robinson]

Created attachment 34356 [details]
Re-order the operations necessary for setting the maximum app cache size

If it's more appropriate to split this into two patches, let me know and I'll be happy to do that.

[Jan Alonzo]

(In reply to comment #4)
> Created an attachment (id=34356) [details]
> Re-order the operations necessary for setting the maximum app cache size
> 
> If it's more appropriate to split this into two patches, let me know and I'll
> be happy to do that.

Maybe add more detail to the ChangeLog?

[Eric Seidel (no email)]

Comment on attachment 34356 [details]
Re-order the operations necessary for setting the maximum app cache size

What I need to know to be able to review this is why changing the order does not cause the layout test to fail on Mac, but does in Gtk?

Perhaps others would have enough context to review this.

[Martin Robinson]

Sorry. Let me try to clarify this. This test failed for me during the GTK+ tests, but has passed for others. I looked over the sqlite docs and it appears that according to the documentation, the order of these operations:

    WebCore::cacheStorage().empty();
    WebCore::cacheStorage().vacuumDatabaseFile();
    WebCore::cacheStorage().setMaximumSize(size);

should really be:

    WebCore::cacheStorage().empty();
    WebCore::cacheStorage().setMaximumSize(size);
    WebCore::cacheStorage().vacuumDatabaseFile();

Making that change caused my GTK+ test to pass. The test passes both before and after the same change is made to the CF port on my Macbook. The CF Mac port seems to be the only other port that does a vacuum when changing the maximum database size. The Qt port also does a .setMaximumSize(...) but never calls vacuum.

I'm new to this, so I'm not sure how I should structure the patch. It's clear that this fixes the test for my version of sqlite, but it's unclear why it was passing in the first place and why it only passes on some versions of sqlite.

[Martin Robinson]

Created attachment 34359 [details]
Patch with more helpful Changelog entries

[Martin Robinson]

Created attachment 34360 [details]
Patch with more helpful Changelog entries

Ack. Attached the wrong patch. Sorry.

[Eric Seidel (no email)]

Comment on attachment 34360 [details]
Patch with more helpful Changelog entries

Ok.  So this looks fine.  Need one more round though.

We need a comment next to the code noting that the order is important.  The ChangeLog (and possibly the code comment) should ideally reference the SQLite docs which say that order matters here.

Again, the change looks totally fine.  The point I'm going for is that we should make sure that we make sure that no one will get this wrong again. :) (When they re-factor the code for instance.)

If you were a committer, I would just r+ this and you could fix it when you land.  Since you aren't, i'm going to ask that you post one more patch.

Thanks again for the patch!

[Martin Robinson]

Looking over the SQLite docs again it seems my original patch is wrong, so I'm glad you r-'d it. I'll do more investigation into why the original test was failing. It may be related to the incomplete GTK+ database API.

[Martin Robinson]

The test was just flaky and I don't see it any longer, so I'm closing this bug.