Bug 195301 - Resource Load Statistics: Log first-party navigations with link decoration
Summary: Resource Load Statistics: Log first-party navigations with link decoration
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: John Wilander
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-03-04 17:18 PST by John Wilander
Modified: 2019-03-07 14:32 PST (History)
8 users (show)

See Also:


Attachments
Patch (25.29 KB, patch)
2019-03-04 17:35 PST, John Wilander
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews123 for ios-simulator-wk2 (10.17 MB, application/zip)
2019-03-04 19:56 PST, Build Bot
no flags Details
Patch (33.26 KB, patch)
2019-03-05 12:45 PST, John Wilander
no flags Details | Formatted Diff | Diff
Patch (34.49 KB, patch)
2019-03-06 12:29 PST, John Wilander
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews123 for ios-simulator-wk2 (2.44 MB, application/zip)
2019-03-06 14:42 PST, Build Bot
no flags Details
Patch (33.20 KB, patch)
2019-03-07 09:57 PST, John Wilander
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John Wilander 2019-03-04 17:18:25 PST
We should add first-party navigations with link decoration to our statistics model and log data to it.
Comment 1 John Wilander 2019-03-04 17:19:00 PST
<rdar://problem/48569971>
Comment 2 John Wilander 2019-03-04 17:35:22 PST
Created attachment 363574 [details]
Patch
Comment 3 Build Bot 2019-03-04 19:56:13 PST
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
Comment 4 Build Bot 2019-03-04 19:56:15 PST
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
Comment 5 John Wilander 2019-03-05 09:22:31 PST
The failing tests seem unrelated. Other patches get them too, for instance https://bugs.webkit.org/show_bug.cgi?id=191645.
Comment 6 John Wilander 2019-03-05 12:45:44 PST
Created attachment 363671 [details]
Patch
Comment 7 John Wilander 2019-03-05 12:47:20 PST
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 8 Brent Fulgham 2019-03-05 18:34:24 PST
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!
Comment 9 John Wilander 2019-03-06 12:29:35 PST
Created attachment 363772 [details]
Patch
Comment 10 John Wilander 2019-03-06 12:31:41 PST
(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!
Comment 11 John Wilander 2019-03-06 13:31:48 PST
ios-sim test failures are unrelated and wpe seems red.
Comment 12 Build Bot 2019-03-06 14:42:43 PST
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
Comment 13 Build Bot 2019-03-06 14:42:45 PST
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
Comment 14 John Wilander 2019-03-06 15:56:19 PST
I checked with Ryan Haddad, and the ios-sim test failure is unrelated.
Comment 15 Brent Fulgham 2019-03-06 16:30:22 PST
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 16 Brent Fulgham 2019-03-06 16:31:24 PST
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!
Comment 17 John Wilander 2019-03-07 09:53:37 PST
(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.
Comment 18 John Wilander 2019-03-07 09:57:01 PST
Created attachment 363888 [details]
Patch
Comment 19 Brent Fulgham 2019-03-07 10:46:07 PST
Comment on attachment 363888 [details]
Patch

r=me
Comment 20 John Wilander 2019-03-07 10:54:11 PST
Comment on attachment 363888 [details]
Patch

Thanks, Brent! We're on a roll here.
Comment 21 WebKit Commit Bot 2019-03-07 11:16:44 PST
Comment on attachment 363888 [details]
Patch

Clearing flags on attachment: 363888

Committed r242603: <https://trac.webkit.org/changeset/242603>
Comment 22 WebKit Commit Bot 2019-03-07 11:16:46 PST
All reviewed patches have been landed.  Closing bug.
Comment 23 Truitt Savell 2019-03-07 13:36:52 PST
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.
Comment 24 Brent Fulgham 2019-03-07 13:45:38 PST
(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?
Comment 25 John Wilander 2019-03-07 13:46:06 PST
Hmm. Nothing failed on EWS. But is the assertion failing. I’ll check and see if I can do a follow-up.
Comment 26 John Wilander 2019-03-07 13:47:05 PST
(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.
Comment 27 Truitt Savell 2019-03-07 13:50:47 PST
(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.
Comment 28 John Wilander 2019-03-07 14:03:50 PST
(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.
Comment 29 John Wilander 2019-03-07 14:32:37 PST
Simple fix up for review in https://bugs.webkit.org/show_bug.cgi?id=195435.