DOMWindow properties may get GC'd before their Window object

• Component: Bindings
• Status: RESOLVED FIXED

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 375345 [details]
Patch

[Ryosuke Niwa]

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.

[Chris Dumez]

Created attachment 375410 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 375410 [details]
Patch

Clearing flags on attachment: 375410

Committed r248155: <https://trac.webkit.org/changeset/248155>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/53866998>

[Darin Adler]

Is there any case where using ImplFrame is correct, now?

[Chris Dumez]

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