Bug 195301

<rdar://problem/48569971>

Created attachment 363574 [details] Patch

Comment on attachment 363574 [details] Patch Attachment 363574 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/11370474 New failing tests: fast/shadow-dom/click-text-inside-linked-slot.html fast/scrolling/ios/hit-testing-iframe-003.html fast/shadow-dom/click-on-slotted-anchor-with-hover.html fast/images/imagemap-polygon-focus-ring.html fast/viewport/ios/device-width-viewport-after-changing-view-scale.html editing/selection/thai-word-at-document-end.html

Created attachment 363588 [details] Archive of layout-test-results from ews123 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6

The failing tests seem unrelated. Other patches get them too, for instance https://bugs.webkit.org/show_bug.cgi?id=191645.

Created attachment 363671 [details] Patch

Brent, I hope you get a chance to look at this since it's the first time I dabble with the DB implementation. Thanks!

Comment on attachment 363671 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=363671&action=review Looks good, with a couple of database issues. If you fix those, I think this will be good to go! > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:78 > +constexpr auto topFrameDecoratedNavigationsFromQuery = "INSERT INTO TopFrameDecoratedNavigationsFrom (fromDomainID, toDomainID) " You also need to create the table. I think you should use a shorter name: maybe NavigationThroughDecoratedLink, with the from and toDomainID as you showed. See the section with CREATE TABLE, below. > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:82 > +constexpr auto updateWasNavigatedToWithLinkDecorationByPrevalentResourceStatementQuery = "UPDATE ObservedDomains SET wasNavigatedToWithLinkDecorationByPrevalentResource = ? WHERE registrableDomain = ?"_s; These names are way too long!

Created attachment 363772 [details] Patch

(In reply to Brent Fulgham from comment #8) > Comment on attachment 363671 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=363671&action=review > > Looks good, with a couple of database issues. If you fix those, I think this > will be good to go! > > > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:78 > > +constexpr auto topFrameDecoratedNavigationsFromQuery = "INSERT INTO TopFrameDecoratedNavigationsFrom (fromDomainID, toDomainID) " > > You also need to create the table. I think you should use a shorter name: > maybe NavigationThroughDecoratedLink, with the from and toDomainID as you > showed. > > See the section with CREATE TABLE, below. Got it. I massaged the names for a while and came up with shorter ones. > > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:82 > > +constexpr auto updateWasNavigatedToWithLinkDecorationByPrevalentResourceStatementQuery = "UPDATE ObservedDomains SET wasNavigatedToWithLinkDecorationByPrevalentResource = ? WHERE registrableDomain = ?"_s; > > These names are way too long! See new patch. Thanks for the comments!

ios-sim test failures are unrelated and wpe seems red.

Comment on attachment 363772 [details] Patch Attachment 363772 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/11401259 New failing tests: fast/viewport/ios/device-width-viewport-after-changing-view-scale.html

Created attachment 363796 [details] Archive of layout-test-results from ews123 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6

I checked with Ryan Haddad, and the ios-sim test failure is unrelated.

Comment on attachment 363772 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=363772&action=review I think this looks right, but I'd suggest getting rid of the new column for 'gotLinkDecorationFromPrevalentResource' in ObservedDomains. It's already modeled in the database separately. > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:83 > +constexpr auto gotLinkDecorationFromPrevalentResourceQuery = "SELECT gotLinkDecorationFromPrevalentResource FROM ObservedDomains WHERE registrableDomain = ?"_s; This is trying to retrieve a column from ObservedDomains that doesn't exist (it would needed to be added to the CREATE for ObservedDomains). However: I think you are properly modeling it as a relationship between the two origins, so an explicit flag is probably not needed in the table. And the fact that we were redirected to by a prevalent domain is already represented as the JOIN of your new table and the ObservedOrigin's existing 'isPrevalent' flag. So I don't think you need these two queries. > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:178 > + , m_gotLinkDecorationFromPrevalentResource(m_database, gotLinkDecorationFromPrevalentResourceQuery) I don't think we need these two statements (and they aren't used!) > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:1007 > + RELEASE_LOG_ERROR(Network, "%p - ResourceLoadStatisticsDatabaseStore::m_updateGotLinkDecorationFromPrevalentResourceStatement failed, error message: %{public}s", this, m_database.lastErrorMsg()); I don't think we want to add this column to ObservedDomains. Instead, I think we want to teach "recursivelyFindNonPrevalentDomainsThatRedirectedToThisDomain" to check for this, or to have our query about whether a resource has been cross-site loaded with link decoration just join the tables, since we know the from ID, and we can look up if one of the fromID's was prevalent. > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.h:181 > + mutable WebCore::SQLiteStatement m_gotLinkDecorationFromPrevalentResource; I don't think we need these two statements. > LayoutTests/http/tests/resourceLoadStatistics/log-cross-site-load-with-link-decoration-expected.txt:12 > + gotLinkDecorationFromPrevalentResource: Yes isPrevalentResource: No Where is this information coming from? dumpResourceLoadStatistics? I haven't made sure that works yet :-)

Comment on attachment 363772 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=363772&action=review In case I wasn't clear, I'd r+ this patch if you removed the statements I was unhappy about. I'll worry about getting the tests to work later. :-) >> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:1007 >> + RELEASE_LOG_ERROR(Network, "%p - ResourceLoadStatisticsDatabaseStore::m_updateGotLinkDecorationFromPrevalentResourceStatement failed, error message: %{public}s", this, m_database.lastErrorMsg()); > > I don't think we want to add this column to ObservedDomains. Instead, I think we want to teach "recursivelyFindNonPrevalentDomainsThatRedirectedToThisDomain" to check for this, or to have our query about whether a resource has been cross-site loaded with link decoration just join the tables, since we know the from ID, and we can look up if one of the fromID's was prevalent. Note: I am not suggesting you do that in this patch!

(In reply to Brent Fulgham from comment #15) > Comment on attachment 363772 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=363772&action=review > > I think this looks right, but I'd suggest getting rid of the new column for > 'gotLinkDecorationFromPrevalentResource' in ObservedDomains. It's already > modeled in the database separately. OK. I guess I'm a little unclear on how the matching between the DB and the in-memory view of things will work. I would like this boolean state to be readily available when I iterate over all statistics but you're right in that it doesn't have to be persisted. > > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:83 > > +constexpr auto gotLinkDecorationFromPrevalentResourceQuery = "SELECT gotLinkDecorationFromPrevalentResource FROM ObservedDomains WHERE registrableDomain = ?"_s; > > This is trying to retrieve a column from ObservedDomains that doesn't exist > (it would needed to be added to the CREATE for ObservedDomains). > > However: I think you are properly modeling it as a relationship between the > two origins, so an explicit flag is probably not needed in the table. And > the fact that we were redirected to by a prevalent domain is already > represented as the JOIN of your new table and the ObservedOrigin's existing > 'isPrevalent' flag. > > So I don't think you need these two queries. Will remove. > > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:178 > > + , m_gotLinkDecorationFromPrevalentResource(m_database, gotLinkDecorationFromPrevalentResourceQuery) > > I don't think we need these two statements (and they aren't used!) Ditto. > > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:1007 > > + RELEASE_LOG_ERROR(Network, "%p - ResourceLoadStatisticsDatabaseStore::m_updateGotLinkDecorationFromPrevalentResourceStatement failed, error message: %{public}s", this, m_database.lastErrorMsg()); > > I don't think we want to add this column to ObservedDomains. Instead, I > think we want to teach > "recursivelyFindNonPrevalentDomainsThatRedirectedToThisDomain" to check for > this, or to have our query about whether a resource has been cross-site > loaded with link decoration just join the tables, since we know the from ID, > and we can look up if one of the fromID's was prevalent. OK, let's implement that later then. > > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.h:181 > > + mutable WebCore::SQLiteStatement m_gotLinkDecorationFromPrevalentResource; > > I don't think we need these two statements. Will remove. > > LayoutTests/http/tests/resourceLoadStatistics/log-cross-site-load-with-link-decoration-expected.txt:12 > > + gotLinkDecorationFromPrevalentResource: Yes isPrevalentResource: No > > Where is this information coming from? dumpResourceLoadStatistics? I haven't > made sure that works yet :-) Yes, it's the convenient dump function. :P Thanks for the comments! New patch coming up.

Created attachment 363888 [details] Patch

Comment on attachment 363888 [details] Patch r=me

Comment on attachment 363888 [details] Patch Thanks, Brent! We're on a roll here.

Comment on attachment 363888 [details] Patch Clearing flags on attachment: 363888 Committed r242603: <https://trac.webkit.org/changeset/242603>

All reviewed patches have been landed. Closing bug.

It looks like the changes in https://trac.webkit.org/changeset/242603/webkit has caused tests on Mac Debug to exit early with 50+ crashes. most of these are in the fast/ directory. build: https://build.webkit.org/builders/Apple%20High%20Sierra%20Debug%20WK2%20%28Tests%29/builds/6947 I was able to reproduce this with command: run-webkit-tests fast/ --debug -f The crashes reproduce in the fast/ directory on 242603 but not on 242602.

(In reply to Truitt Savell from comment #23) > It looks like the changes in https://trac.webkit.org/changeset/242603/webkit > > has caused tests on Mac Debug to exit early with 50+ crashes. most of these > are in the fast/ directory. > > build: > https://build.webkit.org/builders/ > Apple%20High%20Sierra%20Debug%20WK2%20%28Tests%29/builds/6947 > > I was able to reproduce this with command: > > run-webkit-tests fast/ --debug -f > > The crashes reproduce in the fast/ directory on 242603 but not on 242602. John: Looks like logCrossSiteLoadWithLinkDecoration is getting called on the main thread in some cases. Must be missing a dispatch somewhere. Can you take a look please?

Hmm. Nothing failed on EWS. But is the assertion failing. I’ll check and see if I can do a follow-up.

(In reply to Brent Fulgham from comment #24) > (In reply to Truitt Savell from comment #23) > > It looks like the changes in https://trac.webkit.org/changeset/242603/webkit > > > > has caused tests on Mac Debug to exit early with 50+ crashes. most of these > > are in the fast/ directory. > > > > build: > > https://build.webkit.org/builders/ > > Apple%20High%20Sierra%20Debug%20WK2%20%28Tests%29/builds/6947 > > > > I was able to reproduce this with command: > > > > run-webkit-tests fast/ --debug -f > > > > The crashes reproduce in the fast/ directory on 242603 but not on 242602. > > John: Looks like logCrossSiteLoadWithLinkDecoration is getting called on the > main thread in some cases. Must be missing a dispatch somewhere. > > Can you take a look please? I don’t think it’s the thread assertion. It’s the fromDomain != toDomain assertion.

(In reply to John Wilander from comment #25) > Hmm. Nothing failed on EWS. But is the assertion failing. I’ll check and see > if I can do a follow-up. If it only effected Debug WK2 then EWS would not have tested it.

(In reply to Truitt Savell from comment #27) > (In reply to John Wilander from comment #25) > > Hmm. Nothing failed on EWS. But is the assertion failing. I’ll check and see > > if I can do a follow-up. > > If it only effected Debug WK2 then EWS would not have tested it. Got it. I can reproduce locally with for instance fast/block/positioning/absolute-layout-after-image-load.html and will fix it.

Simple fix up for review in https://bugs.webkit.org/show_bug.cgi?id=195435.