Bug 200359 - DOMWindow properties may get GC'd before their Window object
Summary: DOMWindow properties may get GC'd before their Window object
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Bindings (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks: 199517
  Show dependency treegraph
 
Reported: 2019-08-01 14:22 PDT by Chris Dumez
Modified: 2019-08-05 10:16 PDT (History)
7 users (show)

See Also:


Attachments
Patch (14.23 KB, patch)
2019-08-01 14:27 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (14.84 KB, patch)
2019-08-02 08:21 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2019-08-01 14:22:58 PDT
DOMWindow properties may get GC'd before their Window object once their frame is detached. This is unexpected behavior given that these properties persist on the Window after the frame is detached.
Comment 1 Chris Dumez 2019-08-01 14:27:36 PDT
Created attachment 375345 [details]
Patch
Comment 2 Ryosuke Niwa 2019-08-01 22:36:23 PDT
Comment on attachment 375345 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=375345&action=review

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4683
> +            } elsif (GetGenerateIsReachable($interface) eq "ImplWindow") {

Can we use more sensible names like ReachableFromDOMWindow?
I really hate these Impl*. It doesn't explain what it does at all.

> LayoutTests/fast/dom/dom-window-property-gc-after-frame-detach.html:13
> +    w = document.getElementById("testFrame").contentWindow;

Nit: Please don't use one letter variable like w.

> LayoutTests/fast/dom/dom-window-property-gc-after-frame-detach.html:14
> +    for (let p of window_properties)

Dito for p. Also use camelCase?

> LayoutTests/fast/dom/dom-window-property-gc-after-frame-detach.html:17
> +    for (let p of window_properties)

Ditto.

> LayoutTests/fast/dom/dom-window-property-gc-after-frame-detach.html:26
> +        for (let p of window_properties)

Ditto.
Comment 3 Chris Dumez 2019-08-02 08:21:32 PDT
Created attachment 375410 [details]
Patch
Comment 4 WebKit Commit Bot 2019-08-02 09:44:02 PDT
Comment on attachment 375410 [details]
Patch

Clearing flags on attachment: 375410

Committed r248155: <https://trac.webkit.org/changeset/248155>
Comment 5 WebKit Commit Bot 2019-08-02 09:44:04 PDT
All reviewed patches have been landed.  Closing bug.
Comment 6 Radar WebKit Bug Importer 2019-08-02 09:49:13 PDT
<rdar://problem/53866998>
Comment 7 Darin Adler 2019-08-04 11:14:46 PDT
Is there any case where using ImplFrame is correct, now?
Comment 8 Chris Dumez 2019-08-05 08:11:39 PDT
(In reply to Darin Adler from comment #7)
> Is there any case where using ImplFrame is correct, now?

It is only used by Geolocation now. navigator.geolocation never returns null (e.g. when the frame is detached), so I do not think it makes sense to tie to lifetime of its wrapper to the frame. We should tie it to the Window instead. I'll follow-up.