Bug 195301 – Resource Load Statistics: Log first-party navigations with link decoration

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

[John Wilander]

We should add first-party navigations with link decoration to our statistics model and log data to it.

[John Wilander]

<rdar://problem/48569971>

[John Wilander]

Created attachment 363574 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[John Wilander]

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

[John Wilander]

Created attachment 363671 [details]
Patch

[John Wilander]

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

[Brent Fulgham]

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!

[John Wilander]

Created attachment 363772 [details]
Patch

[John Wilander]

(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!

[John Wilander]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[John Wilander]

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

[Brent Fulgham]

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 :-)

[Brent Fulgham]

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!

[John Wilander]

(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.

[John Wilander]

Created attachment 363888 [details]
Patch

[Brent Fulgham]

Comment on attachment 363888 [details]
Patch

r=me

[John Wilander]

Comment on attachment 363888 [details]
Patch

Thanks, Brent! We're on a roll here.

[WebKit Commit Bot]

Comment on attachment 363888 [details]
Patch

Clearing flags on attachment: 363888

Committed r242603: <https://trac.webkit.org/changeset/242603>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Truitt Savell]

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.

[Brent Fulgham]

(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?

[John Wilander]

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

[John Wilander]

(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.

[Truitt Savell]

(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.

[John Wilander]

(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.

[John Wilander]

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