--- /home/yurys/WebKit/WebKitBuild/Release/layout-test-results/inspector/timeline/timeline-event-CancelAnimationFrame-expected.txt +++ /home/yurys/WebKit/WebKitBuild/Release/layout-test-results/inspector/timeline/timeline-event-CancelAnimationFrame-actual.txt @@ -6,7 +6,11 @@ Starting Capture... Evaluating... Stopping Capture... -PASS: Should be 1 AnimationFrameCanceled record. -DETAILS: number -PASS: ScriptTimelineRecord details should be the requestAnimationFrame identifier. +ASSERT: savePageData should have been called in the page before capturing was stopped. +!! EXCEPTION: null is not an object (evaluating 'pageRecordingData.requestAnimationFrameIdentifier') +Stack Trace: #0: (anonymous) (unknown) +#1: asyncFunctionResume [native code] +#2: (anonymous) [native code] +#3: promiseReactionJobWithoutPromise [native code] +#4: promiseReactionJob [native code]
Actually all tests using the same utility resource suffer from the same race: LayoutTests/inspector/timeline/timeline-event-TimerFire.html LayoutTests/inspector/timeline/timeline-event-TimerRemove.html LayoutTests/inspector/timeline/timeline-event-FireAnimationFrame.html LayoutTests/inspector/timeline/timeline-event-EventDispatch.html LayoutTests/inspector/timeline/timeline-event-RequestAnimationFrame.html LayoutTests/inspector/timeline/timeline-event-CancelAnimationFrame.html LayoutTests/inspector/timeline/timeline-event-TimerInstall.html
Created attachment 382354 [details] Patch
Created attachment 382400 [details] Patch
Comment on attachment 382400 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382400&action=review rs=me > LayoutTests/inspector/timeline/resources/timeline-event-utilities.js:9 > + let pageRecordingData = InspectorTest.awaitEvent("SavePageData").then((event) => { NIT: we should call this `savePageDataPromise` since it doesn't actually hold the data anymore. > LayoutTests/inspector/timeline/resources/timeline-event-utilities.js:35 > - .catch(promise.reject); > + .catch((error) => { > + promise.reject(error); > + }); Why was this changed? > LayoutTests/inspector/timeline/resources/timeline-event-utilities.js:43 > + pageRecordingData.then((r) => { > + promise.resolve(r); Once we change the name to `savePageDataPromise`, I'd expect `r` to be renamed to `data`.
Comment on attachment 382400 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382400&action=review >> LayoutTests/inspector/timeline/resources/timeline-event-utilities.js:9 >> + let pageRecordingData = InspectorTest.awaitEvent("SavePageData").then((event) => { > > NIT: we should call this `savePageDataPromise` since it doesn't actually hold the data anymore. Done. >> LayoutTests/inspector/timeline/resources/timeline-event-utilities.js:35 >> + }); > > Why was this changed? As I noted in the changelog there was a bug. It wouldn't work before because 'promise.reject' doesn't preserve a pointer to 'promise' and 'reject' would be called with undefined 'this'. So we either have to 'bind' this or use a closure. >> LayoutTests/inspector/timeline/resources/timeline-event-utilities.js:43 >> + promise.resolve(r); > > Once we change the name to `savePageDataPromise`, I'd expect `r` to be renamed to `data`. Done.
Created attachment 382469 [details] Patch for landing
Comment on attachment 382469 [details] Patch for landing Clearing flags on attachment: 382469 Committed r251856: <https://trac.webkit.org/changeset/251856>
All reviewed patches have been landed. Closing bug.
<rdar://problem/56786459>