Bug 205795 – [WTR] Use short heart beat timer as a TestOption

• Status: RESOLVED
• Resolution: FIXED
• Component: Service Workers

[youenn fablet]

Increase heart beat timer at service worker startup time

[youenn fablet]

Created attachment 386833 [details]
Patch

[youenn fablet]

Created attachment 386842 [details]
Patch

[Alex Christensen]

Could you provide a link to something you think is caused by this timer being too slow?

[youenn fablet]

https://build.webkit.org/results/Apple%20iOS%2013%20Simulator%20Debug%20WK2%20(Tests)/r254057%20(1555)/http/wpt/service-workers/update-service-worker.https-pretty-diff.html is an example.

[Alex Christensen]

Comment on attachment 386842 [details]
Patch

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

> Source/WebCore/workers/service/context/ServiceWorkerThread.h:110
>      Seconds m_heartBeatTimeout { heartBeatTimeout };
> +    Seconds m_heartBeatStartTimeout { heartBeatTimeout };

Your ChangeLog entry indicates that you just replaced heartBeatTimeout's initializer from 60 to 300, but you did something more complicated than that.  What does heartBeatStartTimeout mean?

[youenn fablet]

Created attachment 386950 [details]
Patch

[youenn fablet]

(In reply to Alex Christensen from comment #5)
> Comment on attachment 386842 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=386842&action=review
> 
> > Source/WebCore/workers/service/context/ServiceWorkerThread.h:110
> >      Seconds m_heartBeatTimeout { heartBeatTimeout };
> > +    Seconds m_heartBeatStartTimeout { heartBeatTimeout };
> 
> Your ChangeLog entry indicates that you just replaced heartBeatTimeout's
> initializer from 60 to 300, but you did something more complicated than
> that.  What does heartBeatStartTimeout mean?

Right, I initially did that but this messed up with the sw spinning tests.
I beefed up the change log.

[youenn fablet]

Created attachment 387215 [details]
Patch

[youenn fablet]

(In reply to youenn fablet from comment #8)
> Created attachment 387215 [details]
> Patch

Reduced the patch size.

[Dean Jackson]

Comment on attachment 387215 [details]
Patch

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

> Source/WebCore/workers/service/context/ServiceWorkerThread.cpp:79
> +    , m_heartBeatTimeout(SWContextManager::singleton().connection()->shouldUseShortTimeout() ? heartBeatTimeoutForTest : heartBeatTimeout * 5)

Isn't this a bit confusing? The parameter you pass in isn't what is actually used. Wouldn't it be better to have the calling site pass in a value that is 5x bigger?

[Chris Dumez]

Comment on attachment 387215 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        As seen in the bots, especially slow bots, like ios sim debug, 60 seconds is not always sufficient to start service workers.

Isn't the issue that we're using the "short" timeout of 1 second when running the layout tests and 1 second is simply not enough time sometimes to start a service worker in iOS Debug?

[youenn fablet]

(In reply to Chris Dumez from comment #11)
> Comment on attachment 387215 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=387215&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        As seen in the bots, especially slow bots, like ios sim debug, 60 seconds is not always sufficient to start service workers.
> 
> Isn't the issue that we're using the "short" timeout of 1 second when
> running the layout tests and 1 second is simply not enough time sometimes to
> start a service worker in iOS Debug?

Oh right, let's only use the short timeout values for the spin service worker tests (and disable those tests in iOS sim debug).

[youenn fablet]

Created attachment 387320 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 387320 [details]
Patch

Clearing flags on attachment: 387320

Committed r254341: <https://trac.webkit.org/changeset/254341>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/58480107>