Bug 206681 – REGRESSION: [iOS] http/wpt/cache-storage/quota-third-party.https.html is flaky failing.

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

[Jason Lawrence]

http/wpt/cache-storage/quota-third-party.https.html

Description: 
This test is flaky failing on iOS.

History:
https://results.webkit.org/?limit=5000&suite=layout-tests&test=http%2Fwpt%2Fcache-storage%2Fquota-third-party.https.html

Diff:
--- /Volumes/Data/slave/ios-simulator-13-release-tests-wk2/build/layout-test-results/http/wpt/cache-storage/quota-third-party.https-expected.txt
+++ /Volumes/Data/slave/ios-simulator-13-release-tests-wk2/build/layout-test-results/http/wpt/cache-storage/quota-third-party.https-actual.txt
@@ -1,8 +1,8 @@
-CONSOLE MESSAGE: Cache API operation failed: Quota exceeded
-CONSOLE MESSAGE: Cache API operation failed: Quota exceeded
+CONSOLE MESSAGE: Cache API operation failed: Failed writing data to the file system
+CONSOLE MESSAGE: Cache API operation failed: Internal error
   
 
 PASS same origin iframe has regular quota 
-PASS cross origin iframe has reduced quota 
+FAIL cross origin iframe has reduced quota assert_equals: expected "FAIL" but got "UNEXPECTED"
 PASS cross origin iframe has reduced quota after resetting quota

[Radar WebKit Bug Importer]

<rdar://problem/58842429>

[Ryan Haddad]

This appears to be a recent regression, starting on or around r254397.

[Jason Lawrence]

Created attachment 388925 [details]
Update Test Expectations

[Truitt Savell]

Comment on attachment 388925 [details]
Update Test Expectations

Clearing flags on attachment: 388925

Committed r255185: <https://trac.webkit.org/changeset/255185>

[Kate Cheney]

*** Bug 206857 has been marked as a duplicate of this bug. ***

[Kate Cheney]

Created attachment 389012 [details]
Patch

[youenn fablet]

Comment on attachment 389012 [details]
Patch

r=me but I think we should consider turning off the behavior that removes "websitedata store" for iframes.

View in context: https://bugs.webkit.org/attachment.cgi?id=389012&action=review

> LayoutTests/ChangeLog:9
> +        the origin's cache was deleted before the test finished.

I wonder whether we should turn off this behavior by default for tests and enable it for specific tests that actually require that behavior.
My fear is that other tests might end up have similar issues and we will have difficulties understanding why, or fixing them if WPT.

> LayoutTests/http/wpt/cache-storage/quota-third-party.https.html:13
> +    testRunner.setStatisticsHasHadUserInteraction("https://127.0.0.1:9443", true, start());

Should it be start instead of start()?
Or can we not pass any function at all and not have any start function?

[Kate Cheney]

(In reply to youenn fablet from comment #7)
> Comment on attachment 389012 [details]
> Patch
> 
> r=me but I think we should consider turning off the behavior that removes
> "websitedata store" for iframes.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=389012&action=review
> 
> > LayoutTests/ChangeLog:9
> > +        the origin's cache was deleted before the test finished.
> 
> I wonder whether we should turn off this behavior by default for tests and
> enable it for specific tests that actually require that behavior.
> My fear is that other tests might end up have similar issues and we will
> have difficulties understanding why, or fixing them if WPT.
> 

I agree. Processing ITP domains at random times has caused flaky tests in the past, so I'm wondering if there is an even more general solution that might fix more flakiness than just caches. I'll look into it!

> > LayoutTests/http/wpt/cache-storage/quota-third-party.https.html:13
> > +    testRunner.setStatisticsHasHadUserInteraction("https://127.0.0.1:9443", true, start());
> 
> Should it be start instead of start()?
> Or can we not pass any function at all and not have any start function?

You're right, the start function is not needed. Patch for landing will remove it.

[Kate Cheney]

Created attachment 389024 [details]
Patch for landing

[Kate Cheney]

(In reply to katherine_cheney from comment #8)
> (In reply to youenn fablet from comment #7)
> > Comment on attachment 389012 [details]
> > Patch
> > 
> > r=me but I think we should consider turning off the behavior that removes
> > "websitedata store" for iframes.
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=389012&action=review
> > 
> > > LayoutTests/ChangeLog:9
> > > +        the origin's cache was deleted before the test finished.
> > 
> > I wonder whether we should turn off this behavior by default for tests and
> > enable it for specific tests that actually require that behavior.
> > My fear is that other tests might end up have similar issues and we will
> > have difficulties understanding why, or fixing them if WPT.
> > 
> 
> I agree. Processing ITP domains at random times has caused flaky tests in
> the past, so I'm wondering if there is an even more general solution that
> might fix more flakiness than just caches. I'll look into it!
> 

On the other hand, though, if we are trying to test actual behavior and persistence of website data (i.e. caches, cookies, etc), maybe it could lead to ineffective testing to keep ITP's handling of these off by default in tests, because it won't mimic real situations. ITP's behavior probably shouldn't just be on for ITP tests, because it affects so many other areas of WebKit.

> > > LayoutTests/http/wpt/cache-storage/quota-third-party.https.html:13
> > > +    testRunner.setStatisticsHasHadUserInteraction("https://127.0.0.1:9443", true, start());
> > 
> > Should it be start instead of start()?
> > Or can we not pass any function at all and not have any start function?
> 
> You're right, the start function is not needed. Patch for landing will
> remove it.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 389024 [details]:

editing/spelling/spellcheck-attribute.html bug 206178 (authors: [e-mail], [e-mail], and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 389024 [details]
Patch for landing

Clearing flags on attachment: 389024

Committed r255263: <https://trac.webkit.org/changeset/255263>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[youenn fablet]

> > I agree. Processing ITP domains at random times has caused flaky tests in
> > the past, so I'm wondering if there is an even more general solution that
> > might fix more flakiness than just caches. I'll look into it!

Did these issues show up some bugs in implementation or in tests mainly?

> On the other hand, though, if we are trying to test actual behavior and
> persistence of website data (i.e. caches, cookies, etc), maybe it could lead
> to ineffective testing to keep ITP's handling of these off by default in
> tests, because it won't mimic real situations. ITP's behavior probably
> shouldn't just be on for ITP tests, because it affects so many other areas
> of WebKit.

Layout tests are almost run without any history. For instance, WebsiteDataStore data is removed before each test.
This is very far from a typical situation where ITP would kick in and do useful processing.
I might be wrong but I do not think we get a lot of value there from ITP testing functionality. Maybe we get some threading issues from these tests but wound't be the IPT specific testing be sufficient for that as well.

[Kate Cheney]

(In reply to youenn fablet from comment #14)
> > > I agree. Processing ITP domains at random times has caused flaky tests in
> > > the past, so I'm wondering if there is an even more general solution that
> > > might fix more flakiness than just caches. I'll look into it!
> 
> Did these issues show up some bugs in implementation or in tests mainly?
> 

Only tests (see https://bugs.webkit.org/show_bug.cgi?id=197285 for example). Processing is pretty much always good in implementation.

> > On the other hand, though, if we are trying to test actual behavior and
> > persistence of website data (i.e. caches, cookies, etc), maybe it could lead
> > to ineffective testing to keep ITP's handling of these off by default in
> > tests, because it won't mimic real situations. ITP's behavior probably
> > shouldn't just be on for ITP tests, because it affects so many other areas
> > of WebKit.
> 
> Layout tests are almost run without any history. For instance,
> WebsiteDataStore data is removed before each test.
> This is very far from a typical situation where ITP would kick in and do
> useful processing.
> I might be wrong but I do not think we get a lot of value there from ITP
> testing functionality. Maybe we get some threading issues from these tests
> but wound't be the IPT specific testing be sufficient for that as well.

I see what you're saying, and I agree it could help prevent hours spent looking into a flaky test like I had to do. I'll file a new radar for this. Thanks for your feedback Youenn!