Bug 195422 – Get StorageAccess API features working on SQLite database implementation

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Brent Fulgham]

Make sure that the SQLite database backend works properly with the StorageAccess API.

[Radar WebKit Bug Importer]

<rdar://problem/54213519>

[Kate Cheney]

Created attachment 380595 [details]
Patch

[Brent Fulgham]

Comment on attachment 380595 [details]
Patch

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

Thanks for checking and fixing these.

> Source/WebKit/ChangeLog:10
> +        use the ITP database and uncovered 3 bugs in the process.

Hooray for tests!

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:1164
> +    if (cookieTreatmentResult != CookieTreatmentResult::BlockAndKeep) {

Good. This now matches the behavior in the memory store after we finalized the StorageAccess API behavior.

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:1722
> +    return !!statement.getColumnInt(0) ? StorageAccessPromptWasShown::Yes : StorageAccessPromptWasShown::No;

Oh, whoops! Nice catch.

> LayoutTests/ChangeLog:21
> +        * http/tests/storageAccess/deny-storage-access-under-opener-database-expected.txt: Added.

Using 'svn copy' used to show that these files were just copies of originals, then show a small patch of the change. Did you use git to generate these patches?

[Kate Cheney]

(In reply to Brent Fulgham from comment #3)
> Comment on attachment 380595 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=380595&action=review
> 
> Thanks for checking and fixing these.
> 
> > Source/WebKit/ChangeLog:10
> > +        use the ITP database and uncovered 3 bugs in the process.
> 
> Hooray for tests!
> 
> > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:1164
> > +    if (cookieTreatmentResult != CookieTreatmentResult::BlockAndKeep) {
> 
> Good. This now matches the behavior in the memory store after we finalized
> the StorageAccess API behavior.
> 
> > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:1722
> > +    return !!statement.getColumnInt(0) ? StorageAccessPromptWasShown::Yes : StorageAccessPromptWasShown::No;
> 
> Oh, whoops! Nice catch.
> 
> > LayoutTests/ChangeLog:21
> > +        * http/tests/storageAccess/deny-storage-access-under-opener-database-expected.txt: Added.
> 
> Using 'svn copy' used to show that these files were just copies of
> originals, then show a small patch of the change. Did you use git to
> generate these patches?

I don't think there's a way to do this in git svn :(

[Brent Fulgham]

It looks like the failing test in iOS-wk2 is a flaky timeout, but I don't see failures on iOS-wk2 bots. It's hard to know if this is related since no crash log is generated.

[Kate Cheney]

hmm, strange. I'll investigate locally and see whether I can reproduce the crash. If not, I can try uploading a new patch and see what bots say.

[Brent Fulgham]

(In reply to Brent Fulgham from comment #5)
> It looks like the failing test in iOS-wk2 is a flaky timeout, but I don't
> see failures on iOS-wk2 bots. It's hard to know if this is related since no
> crash log is generated.

It looks like Dave noticed this as well (see Bug 201274).

[Brent Fulgham]

(In reply to Katherine_cheney from comment #6)
> hmm, strange. I'll investigate locally and see whether I can reproduce the
> crash. If not, I can try uploading a new patch and see what bots say.

If you can't reproduce locally, let's proceed with this change. The flakiness dashboard already notes that the test is flaky, but it seems like it's a timeout, not a crash.

Given that there is no crash log, it's hard to know if we are seeing the same behavior (it only indicates that an unexpected message was received by the test).

It is interesting (if true) that your changes affect that media test. It implies that it is related to timing, or perhaps the state of the UIProcess.

If all else fails, you could try running your test cases under an Asan build to see if we have some kind of memory bug that could be contributing. This wouldn't be a bug due to this patch, but there might be an existing bug that you would be uncovering.

[Brent Fulgham]

(In reply to Brent Fulgham from comment #8)
> (In reply to Katherine_cheney from comment #6)
> > hmm, strange. I'll investigate locally and see whether I can reproduce the
> > crash. If not, I can try uploading a new patch and see what bots say.
> 
> If you can't reproduce locally, let's proceed with this change. The
> flakiness dashboard already notes that the test is flaky, but it seems like
> it's a timeout, not a crash.
> 
> Given that there is no crash log, it's hard to know if we are seeing the
> same behavior (it only indicates that an unexpected message was received by
> the test).
> 
> It is interesting (if true) that your changes affect that media test. It
> implies that it is related to timing, or perhaps the state of the UIProcess.
> 
> If all else fails, you could try running your test cases under an Asan build
> to see if we have some kind of memory bug that could be contributing. This
> wouldn't be a bug due to this patch, but there might be an existing bug that
> you would be uncovering.

You could also try uploading a patch without the test cases and see if the media bug persists. It could be that the changes to the test expectation files is doing something here, or just running the new tests is an triggering a latent bug.

[Kate Cheney]

(In reply to Brent Fulgham from comment #9)
> (In reply to Brent Fulgham from comment #8)
> > (In reply to Katherine_cheney from comment #6)
> > > hmm, strange. I'll investigate locally and see whether I can reproduce the
> > > crash. If not, I can try uploading a new patch and see what bots say.
> > 
> > If you can't reproduce locally, let's proceed with this change. The
> > flakiness dashboard already notes that the test is flaky, but it seems like
> > it's a timeout, not a crash.
> > 
> > Given that there is no crash log, it's hard to know if we are seeing the
> > same behavior (it only indicates that an unexpected message was received by
> > the test).
> > 
> > It is interesting (if true) that your changes affect that media test. It
> > implies that it is related to timing, or perhaps the state of the UIProcess.
> > 
> > If all else fails, you could try running your test cases under an Asan build
> > to see if we have some kind of memory bug that could be contributing. This
> > wouldn't be a bug due to this patch, but there might be an existing bug that
> > you would be uncovering.
> 
> You could also try uploading a patch without the test cases and see if the
> media bug persists. It could be that the changes to the test expectation
> files is doing something here, or just running the new tests is an
> triggering a latent bug.

I can't seem to reproduce the crash locally. I'm going to try one more time with the tests in the patch, and if that still causes the crash, I'll try again without them.

[Kate Cheney]

Created attachment 380677 [details]
Patch

[Kate Cheney]

Looks like the iOS tests are all passing now, although not sure if the wincairo failures should be concerning?

[David Kilzer (:ddkilzer)]

Committed r251016: <https://trac.webkit.org/changeset/251016>

[David Kilzer (:ddkilzer)]

(In reply to Katherine_cheney from comment #12)
> Looks like the iOS tests are all passing now, although not sure if the
> wincairo failures should be concerning?

WinCairo bot has other issues (can't apply patches).