Bug 233706 – [Model] Add load and error events to distinguish resource load from model readiness

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Antoine Quint]

As it stands <model> elements have a "ready" promise, which resolves once the resource has been loaded. However, this promise should be used when the <model> is fully ready, and this is done on macOS and iOS asynchronously after the resource has been loaded. So we should have "load" and "error" events and a "complete" property to match <img> to identify resource load, and use the "ready" promise for when the model is fully ready and its various APIs safely usable.

[Radar WebKit Bug Importer]

<rdar://problem/85922697>

[Antoine Quint]

Created attachment 446174 [details]
Patch

[Chris Dumez]

It seems you are not overriding virtualHasPendingActivity() and I believe that is an issue

You need to override it to keep the HTMLModelElement JS wrapper alive as long as you may fire load/error events on it

queueTaskToDispatchEvent() keeps the JS wrapper alive between the call to queueTaskToDispatchEvent() and the point where the event actually gets fired. However, nothing seems to make sure your JS wrapper says alive until you eventually call queueTaskToDispatchEvent().

[Antoine Quint]

Created attachment 446189 [details]
Patch

[Antoine Quint]

(In reply to Chris Dumez from comment #3)
> It seems you are not overriding virtualHasPendingActivity() and I believe
> that is an issue
> 
> You need to override it to keep the HTMLModelElement JS wrapper alive as
> long as you may fire load/error events on it
> 
> queueTaskToDispatchEvent() keeps the JS wrapper alive between the call to
> queueTaskToDispatchEvent() and the point where the event actually gets
> fired. However, nothing seems to make sure your JS wrapper says alive until
> you eventually call queueTaskToDispatchEvent().

Thanks for pointing that out. The new patch has an implementation for virtualHasPendingActivity() and calls suspendIfNeeded() in the create() method.

[Chris Dumez]

Comment on attachment 446189 [details]
Patch

r=me

[Antoine Quint]

Created attachment 446333 [details]
Patch for landing

[Antoine Quint]

Created attachment 446513 [details]
Patch for landing

[Antoine Quint]

Committed r286836 (245069@trunk): <https://commits.webkit.org/245069@trunk>

[Antoine Quint]

A note to bot watchers: the existing test model-element/model-element-contents-layer-updates-with-clipping.html was modified as part of this patch and the iOS-15-Simulator-WK2-Tests-EWS bot saw flakiness with it as part of the run-layout-tests-in-stress-mode step. Subsequent test runs did not see such an issue. I tried to run the test locally with the iPhone 8 simulator using the iOS 12.2 SDK and did not see an issue running the test with `--ios-simulator --release --iterations=100`. I also tried with `--run-singly`. In case you see some flakiness with this test, please mark it as flaky for now and file a bug so that I may investigate.

[Antoine Quint]

That would be the iOS 15.2 SDK, not 12.2.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 234153

[Antoine Quint]

There two types of failures with the changed or new tests. On iOS, you can see some flakiness by running:

run-webkit-tests --no-build --release --ios-simulator --exit-after-n-failures 1 --skip-failing-tests --iterations 100 LayoutTests/model-element/model-element-contents-layer-updates.html LayoutTests/model-element/model-element-contents-layer-updates-with-clipping.html LayoutTests/model-element/model-element-graphics-layers-opacity.html LayoutTests/model-element/model-element-error-and-load-events.html LayoutTests/model-element/model-element-graphics-layers.html LayoutTests/model-element/model-element-ready.html

On macOS, you can see some crashes by running:

rwt --debug --iterations=100 model-element --exit-after-n-failures=1

[Antoine Quint]

Created attachment 447050 [details]
Patch for landing

[Antoine Quint]

Created attachment 449762 [details]
Patch for landing

[EWS]

Committed r288424 (246315@main): <https://commits.webkit.org/246315@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 449762 [details].