Make sure that the SQLite database backend works properly with the StorageAccess API.
<rdar://problem/54213519>
Created attachment 380595 [details] Patch
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?
(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 :(
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.
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.
(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).
(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.
(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.
(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.
Created attachment 380677 [details] Patch
Looks like the iOS tests are all passing now, although not sure if the wincairo failures should be concerning?
Committed r251016: <https://trac.webkit.org/changeset/251016>
(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).