Summary: | REGRESSION (r245756) [Mac] 2 TestWebKitAPI.DownloadProgress* and TestWebKitAPI._WKDownload.DownloadMonitorCancel are flaky timeouts | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Shawn Roberts <sroberts> | ||||||||
Component: | WebKit API | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | aakash_jain, achristensen, ap, beidson, cdumez, commit-queue, david_quesada, jlewis3, rniwa, ryanhaddad, tsavell, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
See Also: |
https://bugs.webkit.org/show_bug.cgi?id=198288 https://bugs.webkit.org/show_bug.cgi?id=197927 |
||||||||||
Attachments: |
|
Description
Shawn Roberts
2019-05-28 09:52:42 PDT
(In reply to Shawn Roberts from comment #0) > It appears that after changes in r245756 the following API tests have started to become flaky timeouts. The timing of when we started seeing these failures matches very well with this commit. Also r245756 seems to modify seems area of the code (DownloadCocoa.mm) as the tests. Maybe we can revert this commit? > I have tried running individual API tests and cannot get a timeout. I have also tried running the entire API test suite a few times and cannot get any timeouts. Maybe worth trying on one of the mac EWS bots (ews150, ews153, ews155, ews119). TestWebKitAPI.DownloadProgress.CancelDownloadWhenProgressIsCanceled is asserting on Mojave debug: TestWebKitAPI.DownloadProgress.CancelDownloadWhenProgressIsCanceled _RegisterApplication(), FAILED TO establish the default connection to the WindowServer, _CGSDefaultConnection() is NULL. ASSERTION FAILED: m_downloads.contains(download.downloadID()) SHOULD NEVER BE REACHED /Volumes/Data/slave/mojave-debug/build/Source/WebKit/NetworkProcess/Downloads/DownloadManager.cpp(174) : void WebKit::DownloadManager::downloadFinished(WebKit::Download &) /Volumes/Data/slave/mojave-debug/build/WebKitBuild/Debug/DerivedSources/WebKit2/NetworkProcessProxyMessageReceiver.cpp(314) : void WebKit::NetworkProcessProxy::didReceiveNetworkProcessProxyMessage(IPC::Connection &, IPC::Decoder &) https://build.webkit.org/builders/Apple%20Mojave%20Debug%20WK2%20(Tests)/builds/2782/steps/run-api-tests/logs/stdio I can reproduce locally with a debug build and the following command: run-api-tests DownloadProgress.LoseProgressWhenDownloadIsCanceled --iterations 25 I cannot reproduce the assertion failure with a build of r245753, but I can with a build of r245756. Created attachment 370906 [details]
Patch
Comment on attachment 370906 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=370906&action=review > Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:629 > + auto resumeDataReference = [resumeData isKindOfClass:[NSData class]] ? IPC::DataReference { static_cast<const uint8_t*>(resumeData.bytes), resumeData.length } : IPC::DataReference { }; Silently failing to use resumeData seems quite unfortunate. What are we losing today, and what will make resuming fail in the future? We should at least assert that resumeData is one of the types known today. (In reply to Alexey Proskuryakov from comment #6) > Comment on attachment 370906 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=370906&action=review > > > Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:629 > > + auto resumeDataReference = [resumeData isKindOfClass:[NSData class]] ? IPC::DataReference { static_cast<const uint8_t*>(resumeData.bytes), resumeData.length } : IPC::DataReference { }; > > Silently failing to use resumeData seems quite unfortunate. I'm not sure I follow what you mean here. Are you referring to the case where resumeData is not an NSData? > What are we losing today, and what will make resuming fail in the future? I'm not aware of any cases where the resumeData will be non-nil, non-NSData (if that's what you mean by what we're "losing"). Though the type check a bit paranoid, I don't necessarily object to it since we're working with an untyped dictionary. > We should at least assert that resumeData is one of the types known today. Sure. I could add an ASSERT(!resumeData || [resumeData isKindOfClass:[NSData class]]) in this method, if that's what you mean. Created attachment 370955 [details]
Patch for landing
Yes. Please consider adding release log too, so that we have a chance of knowing what's causing a problem with resuming in release builds too. Created attachment 370960 [details]
Patch for landing, v2
Added a release log message if the resume data is an unexpected class.
Comment on attachment 370960 [details] Patch for landing, v2 Clearing flags on attachment: 370960 Committed r245901: <https://trac.webkit.org/changeset/245901> All reviewed patches have been landed. Closing bug. |