Bug 243410 – REGRESSION(250037@main): wpt /service-workers/service-worker/registration-updateviacache.https.html has become flaky

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

[Sam Sneddon [:gsnedders]]

See https://wpt.fyi/results/service-workers/service-worker/registration-updateviacache.https.html?label=master&product=safari-15%5Bstable%5D&product=safari%5Bexperimental%5D&aligned&view=subtest

A variety of subtests have regressed:

register-with-updateViaCache-all
register-with-updateViaCache-undefined-then-all
register-with-updateViaCache-imports-then-all
register-with-updateViaCache-none-then-all

https://github.com/WebKit/WebKit/blob/main/LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/registration-updateviacache.https-expected.txt makes it look like our test configuration passes there.

[Radar WebKit Bug Importer]

<rdar://problem/97921749>

[Chris Dumez]

The test is marked as flaky on iOS:
LayoutTests/platform/ios/TestExpectations:webkit.org/b/240074 imported/w3c/web-platform-tests/service-workers/service-worker/registration-updateviacache.https.html [ Pass Failure ]

But seems to be running on macOS.

[Chris Dumez]

Similarly, in STP 150, I see all subtests passing expect the last one. Not sure why the wpt.fyi dashboard is saying differently.

[Chris Dumez]

The flaky failure on iOS (https://bugs.webkit.org/show_bug.cgi?id=240074) looks a lot like the failure here. So I think the test is just flaky on iOS and macOS.

I guess I can try to reproduce the flakiness with iOS instead since our bots seem to indicate it is more prevalent on that platform.

[Chris Dumez]

Previous attempt to fix it by Youenn (Bug 240074) was unsuccessful based on the flakiness dashboard and wpt.fyi results.

[Chris Dumez]

I do see one weirdness in our code here:
```
    // If newestWorker is not null, newestWorker's script url equals job's script url with the exclude fragments
    // flag set, and script's source text is a byte-for-byte match with newestWorker's script resource's source
    // text, then:
    if (newestWorker && equalIgnoringFragmentIdentifier(newestWorker->scriptURL(), job.scriptURL) && newestWorker->type() == job.workerType && result.script == newestWorker->script() && doCertificatesMatch(result.certificateInfo, newestWorker->certificateInfo())) {
```

But the spec says at https://w3c.github.io/ServiceWorker/#start-register (step 5.2):
```
If newestWorker is not null, job’s script url equals newestWorker’s script url, job’s worker type equals newestWorker’s type, and job’s update via cache mode's value equals registration’s update via cache mode, then:

Invoke Resolve Job Promise with job and registration.
Invoke Finish Job with job and abort these steps.
```

It seems we are missing the "update via cache mode" check. I'll try to reproduce the flakiness locally and see if this fixes it.

[Chris Dumez]

(In reply to Chris Dumez from comment #6)
> I do see one weirdness in our code here:
> ```
>     // If newestWorker is not null, newestWorker's script url equals job's
> script url with the exclude fragments
>     // flag set, and script's source text is a byte-for-byte match with
> newestWorker's script resource's source
>     // text, then:
>     if (newestWorker &&
> equalIgnoringFragmentIdentifier(newestWorker->scriptURL(), job.scriptURL) &&
> newestWorker->type() == job.workerType && result.script ==
> newestWorker->script() && doCertificatesMatch(result.certificateInfo,
> newestWorker->certificateInfo())) {
> ```
> 
> But the spec says at https://w3c.github.io/ServiceWorker/#start-register
> (step 5.2):
> ```
> If newestWorker is not null, job’s script url equals newestWorker’s script
> url, job’s worker type equals newestWorker’s type, and job’s update via
> cache mode's value equals registration’s update via cache mode, then:
> 
> Invoke Resolve Job Promise with job and registration.
> Invoke Finish Job with job and abort these steps.
> ```
> 
> It seems we are missing the "update via cache mode" check. I'll try to
> reproduce the flakiness locally and see if this fixes it.

Actually, we do have the updateViaCache check in runRegisterJob() already:
```
        auto* newestWorker = registration->getNewestWorker();
        if (newestWorker && equalIgnoringFragmentIdentifier(job.scriptURL, newestWorker->scriptURL()) && job.workerType == newestWorker->type() && job.registrationOptions.updateViaCache == registration->updateViaCache()) {
            RELEASE_LOG(ServiceWorker, "%p - SWServerJobQueue::runRegisterJob: Found directly reusable registration %llu for job %s (DONE)", this, registration->identifier().toUInt64(), job.identifier().loggingString().utf8().data());
            m_server.resolveRegistrationJob(job, registration->data(), ShouldNotifyWhenResolved::No);
            finishCurrentJob();
            return;
        }
```

[Chris Dumez]

I can't reproduce the flakiness on either macOS or iOS so I am kind of stuck.
I have tried both via WebKitTestRunner and in Safari and did many iterations.

[Chris Dumez]

(In reply to Chris Dumez from comment #8)
> I can't reproduce the flakiness on either macOS or iOS so I am kind of stuck.
> I have tried both via WebKitTestRunner and in Safari and did many iterations.

Ok, I think I have a flaky reproduction now when running the test in Safari using a local http server instead of wpt.fyi.

Since Youenn's original patch was suspecting the memory cache, I tried disabling the memory cache but it didn't fix it.

[Chris Dumez]

(In reply to Chris Dumez from comment #9)
> (In reply to Chris Dumez from comment #8)
> > I can't reproduce the flakiness on either macOS or iOS so I am kind of stuck.
> > I have tried both via WebKitTestRunner and in Safari and did many iterations.
> 
> Ok, I think I have a flaky reproduction now when running the test in Safari
> using a local http server instead of wpt.fyi.
> 
> Since Youenn's original patch was suspecting the memory cache, I tried
> disabling the memory cache but it didn't fix it.

Never mind, I must have tested it wrong earlier. Disabling the memory cache definitely fixes it so Youenn was likely right about the issue. However, it seems his fix was incomplete.

[Chris Dumez]

Pull request: https://github.com/WebKit/WebKit/pull/3034

[EWS]

Committed 253140@main (3f019cf4b5d2): <https://commits.webkit.org/253140@main>

Reviewed commits have been landed. Closing PR #3034 and removing active labels.

[Karl Rackler]

(In reply to Chris Dumez from comment #10)
> (In reply to Chris Dumez from comment #9)
> > (In reply to Chris Dumez from comment #8)
> > > I can't reproduce the flakiness on either macOS or iOS so I am kind of stuck.
> > > I have tried both via WebKitTestRunner and in Safari and did many iterations.
> > 
> > Ok, I think I have a flaky reproduction now when running the test in Safari
> > using a local http server instead of wpt.fyi.
> > 
> > Since Youenn's original patch was suspecting the memory cache, I tried
> > disabling the memory cache but it didn't fix it.
> 
> Never mind, I must have tested it wrong earlier. Disabling the memory cache
> definitely fixes it so Youenn was likely right about the issue. However, it
> seems his fix was incomplete.

imported/w3c/web-platform-tests/service-workers/service-worker/registration-updateviacache.https.html is consistently passing after landing 251742@main with the exception of a one-off on 8/3/2022, 5:10:36 PM at 253089@main for iOS.  

Here is the history with 'Filter expected results' off: 
https://results.webkit.org/?suite=layout-tests&test=imported%2Fw3c%2Fweb-platform-tests%2Fservice-workers%2Fservice-worker%2Fregistration-updateviacache.https.html&platform=ios&platform=mac&limit=50000

[Karl Rackler]

(In reply to Chris Dumez from comment #11)
> Pull request: https://github.com/WebKit/WebKit/pull/3034

After landing 253140@main, Debug is consistently crashing.

History: 
https://results.webkit.org/?suite=layout-tests&test=imported%2Fw3c%2Fweb-platform-tests%2Fservice-workers%2Fservice-worker%2Fregistration-updateviacache.https.html&platform=ios&platform=mac&limit=50000

[Chris Dumez]

(In reply to Karl Rackler from comment #14)
> (In reply to Chris Dumez from comment #11)
> > Pull request: https://github.com/WebKit/WebKit/pull/3034
> 
> After landing 253140@main, Debug is consistently crashing.
> 
> History: 
> https://results.webkit.org/?suite=layout-tests&test=imported%2Fw3c%2Fweb-
> platform-tests%2Fservice-workers%2Fservice-worker%2Fregistration-
> updateviacache.https.html&platform=ios&platform=mac&limit=50000

ASSERTION FAILED: jobData.connectionIdentifier() == Process::identifier()

Looking now.

[Chris Dumez]

(In reply to Chris Dumez from comment #15)
> (In reply to Karl Rackler from comment #14)
> > (In reply to Chris Dumez from comment #11)
> > > Pull request: https://github.com/WebKit/WebKit/pull/3034
> > 
> > After landing 253140@main, Debug is consistently crashing.
> > 
> > History: 
> > https://results.webkit.org/?suite=layout-tests&test=imported%2Fw3c%2Fweb-
> > platform-tests%2Fservice-workers%2Fservice-worker%2Fregistration-
> > updateviacache.https.html&platform=ios&platform=mac&limit=50000
> 
> ASSERTION FAILED: jobData.connectionIdentifier() == Process::identifier()
> 
> Looking now.

Looks like an outdated assertion now that we do the refresh loads in the network process unconditionally. I'll fix shortly.

[Chris Dumez]

(In reply to Chris Dumez from comment #16)
> (In reply to Chris Dumez from comment #15)
> > (In reply to Karl Rackler from comment #14)
> > > (In reply to Chris Dumez from comment #11)
> > > > Pull request: https://github.com/WebKit/WebKit/pull/3034
> > > 
> > > After landing 253140@main, Debug is consistently crashing.
> > > 
> > > History: 
> > > https://results.webkit.org/?suite=layout-tests&test=imported%2Fw3c%2Fweb-
> > > platform-tests%2Fservice-workers%2Fservice-worker%2Fregistration-
> > > updateviacache.https.html&platform=ios&platform=mac&limit=50000
> > 
> > ASSERTION FAILED: jobData.connectionIdentifier() == Process::identifier()
> > 
> > Looking now.
> 
> Looks like an outdated assertion now that we do the refresh loads in the
> network process unconditionally. I'll fix shortly.

Follow-up fix in https://commits.webkit.org/253148@main.