Bug 33733 – ValidityState object should be the same on each access (JS object getting GC'd incorrectly)

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Alexey Proskuryakov]

Created attachment 46707 [details]
test case

Per HTML5, "The validity attribute must return a ValidityState object that represents the validity states of the element. This object is live, and the same object must be returned each time the element's validity attribute is retrieved."

This means that a form element must prevent garbage collection of associated ValidityState wrapper, not just of its underlying DOM object.

[Darin Adler]

Geoff Garen pointed out our usual technique for this sort of thing.

JSHTMLFormControlElement can mark JSValidityState. See JSSharedWorker::markChildren in JSSharedWorkerCustom.cpp.

[Ahmad Saleem]

I am unable to reproduce this bug in Safari 15.5 on macOS 12.4 since now test case shows "PASS". Thanks!

[Ahmad Saleem]

It also matches other browsers - Chrome Canary 104 and Firefox Nightly 103. Thanks!

[Alexey Proskuryakov]

Created attachment 460139 [details]
test case

Sadly, this is not fixed yet, it's just that our garbage collection became less precise than it used to be. This still reproduces when I adjust the test to be more forceful with GC.

[Darin Adler]

A relevant term of art here is "expando properties".

[Ryosuke Niwa]

We should make JSValidityState report its owner element as an opaque root, and make it reachable from the opaque root.

[Ryosuke Niwa]

More precisely, add the following extended attributes to IDL:
    GenerateIsReachable=ImplOwnerNodeRoot,
    GenerateAddOpaqueRoot=opaqueRootConcurrently
And then expose two member functions:
    void* opaqueRootConcurrently() const;
    Node* ownerNode() const;
both of which simply returns asHTMLElement().

[Darin Adler]

That sounds right for how to fix. As for how to test we should have a test for the expando properties on this object in WPT, which already has some other expando property preservation tests.

[Ryosuke Niwa]

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

[EWS]

Committed r295475 (251480@main): <https://commits.webkit.org/251480@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/94897805>