Bug 193213 – Fix Resource Timing buffer edge cases for WPT

• Component: New Bugs
• Status: RESOLVED FIXED

Fix Resource Timing buffer edge cases for WPT

Created attachment 358530 [details]
Patch

Created attachment 358532 [details]
Patch

This patch fixes:
https://bugs.webkit.org/show_bug.cgi?id=193004
https://bugs.webkit.org/show_bug.cgi?id=193006

[EWS Watchlist]

Comment on attachment 358532 [details]
Patch

Attachment 358532 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10663037

New failing tests:
http/tests/performance/performance-resource-timing-resourcetimingbufferfull-shrinking-buffer-crash.html

[EWS Watchlist]

Created attachment 358540 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 358532 [details]
Patch

Attachment 358532 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/10663138

New failing tests:
http/tests/performance/performance-resource-timing-resourcetimingbufferfull-shrinking-buffer-crash.html

[EWS Watchlist]

Created attachment 358544 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 358532 [details]
Patch

Attachment 358532 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/10663410

New failing tests:
http/tests/performance/performance-resource-timing-resourcetimingbufferfull-shrinking-buffer-crash.html

[EWS Watchlist]

Created attachment 358552 [details]
Archive of layout-test-results from ews114 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 358532 [details]
Patch

Attachment 358532 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/10664196

New failing tests:
http/tests/performance/performance-resource-timing-resourcetimingbufferfull-shrinking-buffer-crash.html

[EWS Watchlist]

Created attachment 358554 [details]
Archive of layout-test-results from ews201 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews201  Port: win-future  Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit

[EWS Watchlist]

Comment on attachment 358532 [details]
Patch

Attachment 358532 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10663761

New failing tests:
http/tests/performance/performance-resource-timing-resourcetimingbufferfull-shrinking-buffer-crash.html

[EWS Watchlist]

Created attachment 358555 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[youenn fablet]

Comment on attachment 358532 [details]
Patch

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

> Source/WebCore/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=193213

It seems one WebKit test needs rebasing.

> Source/WebCore/ChangeLog:13
> +        buffer-full-when-populate-entries.html

Should we import them in WebKit (you can do Tools/Scripts/import-w3c-tests -t web-platform-tests/resource-timing for that purpose)?

> Source/WebCore/ChangeLog:18
> +        (WebCore::Performance::resourceTimingBufferFullTimerFired):

Could you describe a little bit the changes?

Created attachment 360247 [details]
Patch

[EWS Watchlist]

Comment on attachment 360247 [details]
Patch

Attachment 360247 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10904194

New failing tests:
http/tests/performance/performance-resource-timing-resourcetimingbufferfull-shrinking-buffer-crash.html

[EWS Watchlist]

Created attachment 360249 [details]
Archive of layout-test-results from ews100 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 360247 [details]
Patch

Attachment 360247 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/10904210

New failing tests:
http/tests/performance/performance-resource-timing-resourcetimingbufferfull-shrinking-buffer-crash.html

[EWS Watchlist]

Created attachment 360250 [details]
Archive of layout-test-results from ews107 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 360247 [details]
Patch

Attachment 360247 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/10904292

New failing tests:
http/tests/performance/performance-resource-timing-resourcetimingbufferfull-shrinking-buffer-crash.html

[EWS Watchlist]

Created attachment 360253 [details]
Archive of layout-test-results from ews200 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[EWS Watchlist]

Comment on attachment 360247 [details]
Patch

Attachment 360247 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/10904244

New failing tests:
http/tests/performance/performance-resource-timing-resourcetimingbufferfull-shrinking-buffer-crash.html

[EWS Watchlist]

Created attachment 360254 [details]
Archive of layout-test-results from ews114 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 360247 [details]
Patch

Attachment 360247 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10904275

New failing tests:
imported/w3c/web-platform-tests/resource-timing/buffer-full-add-after-full-event.html
http/tests/performance/performance-resource-timing-resourcetimingbufferfull-shrinking-buffer-crash.html

[EWS Watchlist]

Created attachment 360255 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

Created attachment 360264 [details]
Patch

[EWS Watchlist]

Comment on attachment 360264 [details]
Patch

Attachment 360264 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10907141

New failing tests:
imported/w3c/web-platform-tests/resource-timing/buffer-full-add-after-full-event.html

[EWS Watchlist]

Created attachment 360268 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

Created attachment 360344 [details]
Patch

Created attachment 360345 [details]
Patch

[youenn fablet]

Comment on attachment 360345 [details]
Patch

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

> Source/WebCore/page/Performance.cpp:249
> +        if (beforeCount <= afterCount) {

If this check is true, it seems we would be loosing some information, how big an issue is it?
Should we do a smarter check here?
Something like, continue if we ever queue any entry. Or is there a risk with that approach?

> LayoutTests/imported/w3c/web-platform-tests/resource-timing/redirects.sub-expected.txt:7
> +FAIL Testing resource entries assert_unreached: http://localhost:8800/common/redirect.py?location=https://www.localhost:9443/resource-timing/resources/blank_page_green.htm?id=xhr is expected to be in the Resource Timing buffer Reached unreachable code

We probably could rewrite the test (as a WPT PR) to make www.localhost be 127.0.0.1 in that context.
Ditto below.

[Yoav Weiss]

Comment on attachment 360345 [details]
Patch

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

>> Source/WebCore/page/Performance.cpp:249
>> +        if (beforeCount <= afterCount) {
> 
> If this check is true, it seems we would be loosing some information, how big an issue is it?
> Should we do a smarter check here?
> Something like, continue if we ever queue any entry. Or is there a risk with that approach?

In the spec discussions, it was agreed (with rniwa) that this is the desired behaviour. If the event fired, but its handler did not increase the buffer size or cleared the entries, running the event again (because there are entries in the secondary buffer) is likely to result in an infinite loop.
So, in this rare case when there's a handler which doesn't make any room in the buffer, we will lose entries. That seems reasonable.

[Ryosuke Niwa]

Yeah, in this particular case, keep firing the event is probably no that useful. In particular, we don't want to keep the buffer full in the event the page wasn't listening to the event at all.

Created attachment 362013 [details]
Patch

Created attachment 362014 [details]
Patch

[EWS Watchlist]

Comment on attachment 362014 [details]
Patch

Attachment 362014 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/11147120

New failing tests:
http/tests/performance/performance-resource-timing-resourcetimingbufferfull-shrinking-buffer-crash.html

[EWS Watchlist]

Created attachment 362020 [details]
Archive of layout-test-results from ews100 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 362014 [details]
Patch

Attachment 362014 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11147143

New failing tests:
http/tests/performance/performance-resource-timing-resourcetimingbufferfull-shrinking-buffer-crash.html

[EWS Watchlist]

Created attachment 362021 [details]
Archive of layout-test-results from ews107 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 362014 [details]
Patch

Attachment 362014 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/11147189

New failing tests:
http/tests/performance/performance-resource-timing-resourcetimingbufferfull-shrinking-buffer-crash.html

[EWS Watchlist]

Created attachment 362024 [details]
Archive of layout-test-results from ews113 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 362014 [details]
Patch

Attachment 362014 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/11147464

New failing tests:
http/tests/performance/performance-resource-timing-resourcetimingbufferfull-shrinking-buffer-crash.html

[EWS Watchlist]

Created attachment 362025 [details]
Archive of layout-test-results from ews201 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews201  Port: win-future  Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit

Created attachment 362028 [details]
Patch

[EWS Watchlist]

Comment on attachment 362028 [details]
Patch

Attachment 362028 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/11148272

New failing tests:
http/tests/performance/performance-resource-timing-resourcetimingbufferfull-shrinking-buffer-crash.html

[EWS Watchlist]

Created attachment 362034 [details]
Archive of layout-test-results from ews102 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-highsierra  Platform: Mac OS X 10.13.6

Created attachment 362039 [details]
Patch

[EWS Watchlist]

Comment on attachment 362039 [details]
Patch

Attachment 362039 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11150028

New failing tests:
imported/w3c/web-platform-tests/resource-timing/buffer-full-add-after-full-event.html

[EWS Watchlist]

Created attachment 362060 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

Created attachment 362095 [details]
Patch

Created attachment 362115 [details]
Patch

@youennf can you PTAL? :)

[WebKit Commit Bot]

Comment on attachment 362115 [details]
Patch

Clearing flags on attachment: 362115

Committed r242209: <https://trac.webkit.org/changeset/242209>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/48481194>