Bug 203625 – [GTK] Web Inspector: inspector/timeline/timeline-event-*.html are failing

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Yury Semikhatsky]

--- /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]

[Yury Semikhatsky]

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

[Yury Semikhatsky]

Created attachment 382354 [details]
Patch

[Yury Semikhatsky]

Created attachment 382400 [details]
Patch

[Devin Rousso]

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`.

[Yury Semikhatsky]

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.

[Yury Semikhatsky]

Created attachment 382469 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 382469 [details]
Patch for landing

Clearing flags on attachment: 382469

Committed r251856: <https://trac.webkit.org/changeset/251856>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/56786459>