Bug 33733 - ValidityState object should be the same on each access (JS object getting GC'd incorrectly)
Summary: ValidityState object should be the same on each access (JS object getting GC'...
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Ryosuke Niwa
Keywords: InRadar
Depends on:
Reported: 2010-01-15 13:15 PST by Alexey Proskuryakov
Modified: 2022-06-11 15:34 PDT (History)
8 users (show)

See Also:

test case (496 bytes, text/html)
2010-01-15 13:15 PST, Alexey Proskuryakov
no flags Details
test case (565 bytes, text/html)
2022-06-09 10:55 PDT, Alexey Proskuryakov
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 2010-01-15 13:15:54 PST
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.
Comment 1 Darin Adler 2010-01-16 13:31:28 PST
Geoff Garen pointed out our usual technique for this sort of thing.

JSHTMLFormControlElement can mark JSValidityState. See JSSharedWorker::markChildren in JSSharedWorkerCustom.cpp.
Comment 2 Ahmad Saleem 2022-06-08 15:36:39 PDT
I am unable to reproduce this bug in Safari 15.5 on macOS 12.4 since now test case shows "PASS". Thanks!
Comment 3 Ahmad Saleem 2022-06-08 15:37:35 PDT
It also matches other browsers - Chrome Canary 104 and Firefox Nightly 103. Thanks!
Comment 4 Alexey Proskuryakov 2022-06-09 10:55:12 PDT
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.
Comment 5 Darin Adler 2022-06-09 13:11:28 PDT
A relevant term of art here is "expando properties".
Comment 6 Ryosuke Niwa 2022-06-10 20:41:51 PDT
We should make JSValidityState report its owner element as an opaque root, and make it reachable from the opaque root.
Comment 7 Ryosuke Niwa 2022-06-10 20:48:25 PDT
More precisely, add the following extended attributes to IDL:
And then expose two member functions:
    void* opaqueRootConcurrently() const;
    Node* ownerNode() const;
both of which simply returns asHTMLElement().
Comment 8 Darin Adler 2022-06-11 09:41:06 PDT
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.
Comment 9 Ryosuke Niwa 2022-06-11 13:09:24 PDT
Pull request: https://github.com/WebKit/WebKit/pull/1460
Comment 10 EWS 2022-06-11 15:33:34 PDT
Committed r295475 (251480@main): <https://commits.webkit.org/251480@main>

Reviewed commits have been landed. Closing PR #1460 and removing active labels.
Comment 11 Radar WebKit Bug Importer 2022-06-11 15:34:14 PDT