Bug 206672 - REGRESSION: [iOS release] http/tests/security/window-named-proto.html is a flaky timing out
Summary: REGRESSION: [iOS release] http/tests/security/window-named-proto.html is a fl...
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: Jiewen Tan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-01-23 10:06 PST by Jason Lawrence
Modified: 2020-01-31 01:07 PST (History)
10 users (show)

See Also:


Attachments
Patch (1.59 KB, patch)
2020-01-30 14:05 PST, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (1.61 KB, patch)
2020-01-30 14:09 PST, Jiewen Tan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jason Lawrence 2020-01-23 10:06:43 PST
http/tests/security/window-named-proto.html

Description:
This test is timing out. It looks like this regressed around 252941. I have not tried to reproduce this.

History:
https://results.webkit.org/?limit=50000&suite=layout-tests&test=http%2Ftests%2Fsecurity%2Fwindow-named-proto.html

Diff:
--- /Volumes/Data/slave/ios-simulator-13-release-tests-wk2/build/layout-test-results/http/tests/security/window-named-proto-expected.txt
+++ /Volumes/Data/slave/ios-simulator-13-release-tests-wk2/build/layout-test-results/http/tests/security/window-named-proto-actual.txt
@@ -1,2 +1,3 @@
-CONSOLE MESSAGE: line 2: TypeError: null is not an object (evaluating 'document.body.innerHTML')
+#PID UNRESPONSIVE - WebKitTestRunnerApp (pid 29959)
+FAIL: Timed out waiting for notifyDone to be called
Comment 1 Radar WebKit Bug Importer 2020-01-23 10:07:09 PST
<rdar://problem/58838583>
Comment 2 Truitt Savell 2020-01-24 09:35:25 PST
This is a highly intermittent timeout on iOS iOS Release
Comment 3 Alexey Proskuryakov 2020-01-24 23:20:55 PST
When I increase history limit, I see even older timeouts, starting with r251319. It is pretty rare, so history doesn't point at the culprit, but this is sort of consistent with HTML event loop changes.
Comment 4 Jiewen Tan 2020-01-30 14:05:27 PST
Created attachment 389286 [details]
Patch
Comment 5 Jiewen Tan 2020-01-30 14:09:40 PST
Created attachment 389288 [details]
Patch
Comment 6 Ryosuke Niwa 2020-01-30 14:33:53 PST
Comment on attachment 389288 [details]
Patch

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

> Source/WebCore/platform/network/DataURLDecoder.cpp:46
> -    static auto& queue = WorkQueue::create("org.webkit.DataURLDecoder").leakRef();
> +    static auto& queue = WorkQueue::create("org.webkit.DataURLDecoder", WorkQueue::Type::Serial, WorkQueue::QOS::UserInitiated).leakRef();

Hm... what's the default QOS? I thought all threads we start will be at least user initiated by default?
Comment 7 Jiewen Tan 2020-01-30 14:44:29 PST
(In reply to Ryosuke Niwa from comment #6)
> Comment on attachment 389288 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=389288&action=review
> 
> > Source/WebCore/platform/network/DataURLDecoder.cpp:46
> > -    static auto& queue = WorkQueue::create("org.webkit.DataURLDecoder").leakRef();
> > +    static auto& queue = WorkQueue::create("org.webkit.DataURLDecoder", WorkQueue::Type::Serial, WorkQueue::QOS::UserInitiated).leakRef();
> 
> Hm... what's the default QOS? I thought all threads we start will be at
> least user initiated by default?

static Ref<WorkQueue> create(const char* name, Type = Type::Serial, QOS = QOS::Default);
Comment 8 Chris Dumez 2020-01-30 14:57:46 PST
Comment on attachment 389288 [details]
Patch

Makes sense to use user-initiated IMO since this is the priority on the WebContent process' main thread and since this thread getting pre-empted would slow down a load, which would be visible to the user.
Comment 9 Ryosuke Niwa 2020-01-30 15:11:38 PST
(In reply to Chris Dumez from comment #8)
> Comment on attachment 389288 [details]
> Patch
> 
> Makes sense to use user-initiated IMO since this is the priority on the
> WebContent process' main thread and since this thread getting pre-empted
> would slow down a load, which would be visible to the user.

FWIW, I'm pretty sure WebContent's main thread runs at user interactive QOS, which is slightly higher priority than user-initiated QOS.
Comment 10 Chris Dumez 2020-01-30 15:18:17 PST
(In reply to Ryosuke Niwa from comment #9)
> (In reply to Chris Dumez from comment #8)
> > Comment on attachment 389288 [details]
> > Patch
> > 
> > Makes sense to use user-initiated IMO since this is the priority on the
> > WebContent process' main thread and since this thread getting pre-empted
> > would slow down a load, which would be visible to the user.
> 
> FWIW, I'm pretty sure WebContent's main thread runs at user interactive QOS,
> which is slightly higher priority than user-initiated QOS.

Not 100% sure about that, would need to be validated on device to be sure. Either way, I do think that using user-initiated for data url is better than Default. I think it would be riskier to use UserInteractive.
Comment 11 Jiewen Tan 2020-01-31 00:23:53 PST
Comment on attachment 389288 [details]
Patch

Chris, thanks for r+ this patch.
Comment 12 WebKit Commit Bot 2020-01-31 01:07:10 PST
Comment on attachment 389288 [details]
Patch

Clearing flags on attachment: 389288

Committed r255487: <https://trac.webkit.org/changeset/255487>
Comment 13 WebKit Commit Bot 2020-01-31 01:07:12 PST
All reviewed patches have been landed.  Closing bug.