Skip to content

Commit b39bc89

Browse files
cdumezcdumez
committed
navigator.geolocation wrapper should not become GC-collectable once its frame is detached
https://bugs.webkit.org/show_bug.cgi?id=200436 Reviewed by Darin Adler. Source/WebCore: navigator.geolocation wrapper should not become GC-collectable once its frame is detached, given that it can outlive the frame. Instead, tie the navigator.geolocation wrapper's lifetime to its Navigator's. Test: fast/dom/navigator-property-gc-after-frame-detach.html * Modules/geolocation/Geolocation.cpp: (WebCore::Geolocation::create): (WebCore::Geolocation::Geolocation): (WebCore::Geolocation::navigator): (WebCore::Geolocation::frame const): * Modules/geolocation/Geolocation.h: * Modules/geolocation/Geolocation.idl: * Modules/geolocation/NavigatorGeolocation.cpp: (WebCore::NavigatorGeolocation::NavigatorGeolocation): (WebCore::NavigatorGeolocation::from): (WebCore::NavigatorGeolocation::geolocation): (WebCore::NavigatorGeolocation::geolocation const): * Modules/geolocation/NavigatorGeolocation.h: * bindings/js/JSNavigatorCustom.cpp: (WebCore::JSNavigator::visitAdditionalChildren): * bindings/js/JSWorkerNavigatorCustom.cpp: (WebCore::JSWorkerNavigator::visitAdditionalChildren): * bindings/scripts/CodeGeneratorJS.pm: (GenerateImplementation): * bindings/scripts/IDLAttributes.json: * page/Navigator.cpp: (WebCore::Navigator::plugins): (WebCore::Navigator::mimeTypes): * page/NavigatorBase.h: * plugins/DOMMimeTypeArray.cpp: (WebCore::DOMMimeTypeArray::DOMMimeTypeArray): * plugins/DOMMimeTypeArray.h: * plugins/DOMMimeTypeArray.idl: * plugins/DOMPluginArray.cpp: (WebCore::DOMPluginArray::DOMPluginArray): * plugins/DOMPluginArray.h: * plugins/DOMPluginArray.idl: * workers/service/ServiceWorkerContainer.h: * workers/service/ServiceWorkerContainer.idl: LayoutTests: Add layout test coverage. * fast/dom/navigator-property-gc-after-frame-detach-expected.txt: Added. * fast/dom/navigator-property-gc-after-frame-detach.html: Added. Canonical link: https://commits.webkit.org/214214@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@248276 268f45cc-cd09-0410-ab3c-d52691b4dbfc
1 parent 4c75125 commit b39bc89

27 files changed

Lines changed: 243 additions & 71 deletions

‎LayoutTests/ChangeLog‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,15 @@
1+
2019-08-05 Chris Dumez <[email protected]>
2+
3+
navigator.geolocation wrapper should not become GC-collectable once its frame is detached
4+
https://bugs.webkit.org/show_bug.cgi?id=200436
5+
6+
Reviewed by Darin Adler.
7+
8+
Add layout test coverage.
9+
10+
* fast/dom/navigator-property-gc-after-frame-detach-expected.txt: Added.
11+
* fast/dom/navigator-property-gc-after-frame-detach.html: Added.
12+
113
2019-08-05 Devin Rousso <[email protected]>
214

315
Web Inspector: rename "Stylesheet" to "Style Sheet" to match spec text

‎LayoutTests/fast/dom/navigator-property-gc-after-frame-detach-expected.txt‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
Tests that Navigator properties do not get GC'd before their Navigator object.
2+
3+
On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
4+
5+
6+
PASS frameNavigator.geolocation.foo is 1
7+
PASS frameNavigator.mimeTypes.foo is 1
8+
PASS frameNavigator.plugins.foo is 1
9+
PASS frameNavigator.serviceWorker.foo is 1
10+
11+
PASS frameNavigator.geolocation.foo is 1
12+
PASS frameNavigator.mimeTypes.foo is 1
13+
PASS frameNavigator.plugins.foo is 1
14+
PASS frameNavigator.serviceWorker.foo is 1
15+
16+
PASS frameNavigator.geolocation.foo is 1
17+
PASS frameNavigator.mimeTypes.foo is 1
18+
PASS frameNavigator.plugins.foo is 1
19+
PASS frameNavigator.serviceWorker.foo is 1
20+
PASS successfullyParsed is true
21+
22+
TEST COMPLETE
23+

‎LayoutTests/fast/dom/navigator-property-gc-after-frame-detach.html‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
<!DOCTYPE html>
2+
<html>
3+
<body>
4+
<script src="../../resources/js-test.js"></script>
5+
<iframe id="testFrame" src="about:blank"></iframe>
6+
<script>
7+
description("Tests that Navigator properties do not get GC'd before their Navigator object.");
8+
jsTestIsAsync = true;
9+
10+
var navigatorProperties = [ "geolocation", "mimeTypes", "plugins" ];
11+
if (navigator.serviceWorker)
12+
navigatorProperties.push("serviceWorker");
13+
14+
onload = function() {
15+
frameNavigator = document.getElementById("testFrame").contentWindow.navigator;
16+
for (let navigatorProperty of navigatorProperties)
17+
eval("frameNavigator." + navigatorProperty + ".foo = 1;");
18+
document.getElementById("testFrame").remove();
19+
for (let navigatorProperty of navigatorProperties)
20+
shouldBe("frameNavigator." + navigatorProperty + ".foo", "1");
21+
debug("");
22+
gc();
23+
for (let navigatorProperty of navigatorProperties)
24+
shouldBe("frameNavigator." + navigatorProperty + ".foo", "1");
25+
debug("");
26+
setTimeout(() => {
27+
gc();
28+
for (let navigatorProperty of navigatorProperties)
29+
shouldBe("frameNavigator." + navigatorProperty + ".foo", "1");
30+
finishJSTest();
31+
}, 10);
32+
}
33+
</script>
34+
</body>

‎LayoutTests/platform/mac-wk1/fast/dom/navigator-property-gc-after-frame-detach-expected.txt‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
Tests that Navigator properties do not get GC'd before their Navigator object.
2+
3+
On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
4+
5+
6+
PASS frameNavigator.geolocation.foo is 1
7+
PASS frameNavigator.mimeTypes.foo is 1
8+
PASS frameNavigator.plugins.foo is 1
9+
10+
PASS frameNavigator.geolocation.foo is 1
11+
PASS frameNavigator.mimeTypes.foo is 1
12+
PASS frameNavigator.plugins.foo is 1
13+
14+
PASS frameNavigator.geolocation.foo is 1
15+
PASS frameNavigator.mimeTypes.foo is 1
16+
PASS frameNavigator.plugins.foo is 1
17+
PASS successfullyParsed is true
18+
19+
TEST COMPLETE
20+

‎LayoutTests/platform/win/fast/dom/navigator-property-gc-after-frame-detach-expected.txt‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
Tests that Navigator properties do not get GC'd before their Navigator object.
2+
3+
On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
4+
5+
6+
PASS frameNavigator.geolocation.foo is 1
7+
PASS frameNavigator.mimeTypes.foo is 1
8+
PASS frameNavigator.plugins.foo is 1
9+
10+
PASS frameNavigator.geolocation.foo is 1
11+
PASS frameNavigator.mimeTypes.foo is 1
12+
PASS frameNavigator.plugins.foo is 1
13+
14+
PASS frameNavigator.geolocation.foo is 1
15+
PASS frameNavigator.mimeTypes.foo is 1
16+
PASS frameNavigator.plugins.foo is 1
17+
PASS successfullyParsed is true
18+
19+
TEST COMPLETE
20+

‎LayoutTests/platform/wk2/TestExpectations‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -160,8 +160,6 @@ plugins/npruntime/embed-property-iframe-equality.html
160160

161161
webkit.org/b/105952 fast/loader/submit-form-while-parsing-2.html [ Pass Failure ]
162162

163-
webkit.org/b/141122 editing/selection/programmatic-selection-on-mac-is-directionless.html [ Pass Failure ]
164-
165163
webkit.org/b/149087 http/tests/cache/disk-cache/disk-cache-cancel.html [ Pass Failure ]
166164

167165
http/tests/appcache/decide-navigation-policy-after-delay.html [ Pass ]

‎Source/WebCore/ChangeLog‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,51 @@
1+
2019-08-05 Chris Dumez <[email protected]>
2+
3+
navigator.geolocation wrapper should not become GC-collectable once its frame is detached
4+
https://bugs.webkit.org/show_bug.cgi?id=200436
5+
6+
Reviewed by Darin Adler.
7+
8+
navigator.geolocation wrapper should not become GC-collectable once its frame is detached, given
9+
that it can outlive the frame. Instead, tie the navigator.geolocation wrapper's lifetime to its
10+
Navigator's.
11+
12+
Test: fast/dom/navigator-property-gc-after-frame-detach.html
13+
14+
* Modules/geolocation/Geolocation.cpp:
15+
(WebCore::Geolocation::create):
16+
(WebCore::Geolocation::Geolocation):
17+
(WebCore::Geolocation::navigator):
18+
(WebCore::Geolocation::frame const):
19+
* Modules/geolocation/Geolocation.h:
20+
* Modules/geolocation/Geolocation.idl:
21+
* Modules/geolocation/NavigatorGeolocation.cpp:
22+
(WebCore::NavigatorGeolocation::NavigatorGeolocation):
23+
(WebCore::NavigatorGeolocation::from):
24+
(WebCore::NavigatorGeolocation::geolocation):
25+
(WebCore::NavigatorGeolocation::geolocation const):
26+
* Modules/geolocation/NavigatorGeolocation.h:
27+
* bindings/js/JSNavigatorCustom.cpp:
28+
(WebCore::JSNavigator::visitAdditionalChildren):
29+
* bindings/js/JSWorkerNavigatorCustom.cpp:
30+
(WebCore::JSWorkerNavigator::visitAdditionalChildren):
31+
* bindings/scripts/CodeGeneratorJS.pm:
32+
(GenerateImplementation):
33+
* bindings/scripts/IDLAttributes.json:
34+
* page/Navigator.cpp:
35+
(WebCore::Navigator::plugins):
36+
(WebCore::Navigator::mimeTypes):
37+
* page/NavigatorBase.h:
38+
* plugins/DOMMimeTypeArray.cpp:
39+
(WebCore::DOMMimeTypeArray::DOMMimeTypeArray):
40+
* plugins/DOMMimeTypeArray.h:
41+
* plugins/DOMMimeTypeArray.idl:
42+
* plugins/DOMPluginArray.cpp:
43+
(WebCore::DOMPluginArray::DOMPluginArray):
44+
* plugins/DOMPluginArray.h:
45+
* plugins/DOMPluginArray.idl:
46+
* workers/service/ServiceWorkerContainer.h:
47+
* workers/service/ServiceWorkerContainer.idl:
48+
149
2019-08-05 Andy Estes <[email protected]>
250

351
[WebIDL] Support partial dictionaries and conditional dictionary members

‎Source/WebCore/Modules/geolocation/Geolocation.cpp‎

Lines changed: 16 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@
3838
#include "GeolocationError.h"
3939
#include "GeolocationPosition.h"
4040
#include "Geoposition.h"
41+
#include "Navigator.h"
4142
#include "Page.h"
4243
#include "PositionError.h"
4344
#include "RuntimeApplicationChecks.h"
@@ -129,18 +130,16 @@ void Geolocation::Watchers::getNotifiersVector(GeoNotifierVector& copy) const
129130
copy = copyToVector(m_idToNotifierMap.values());
130131
}
131132

132-
Ref<Geolocation> Geolocation::create(ScriptExecutionContext* context)
133+
Ref<Geolocation> Geolocation::create(Navigator& navigator)
133134
{
134-
auto geolocation = adoptRef(*new Geolocation(context));
135+
auto geolocation = adoptRef(*new Geolocation(navigator));
135136
geolocation.get().suspendIfNeeded();
136137
return geolocation;
137138
}
138139

139-
Geolocation::Geolocation(ScriptExecutionContext* context)
140-
: ActiveDOMObject(context)
141-
, m_allowGeolocation(Unknown)
142-
, m_isSuspended(false)
143-
, m_hasChangedPosition(false)
140+
Geolocation::Geolocation(Navigator& navigator)
141+
: ActiveDOMObject(navigator.scriptExecutionContext())
142+
, m_navigator(makeWeakPtr(navigator))
144143
, m_resumeTimer(*this, &Geolocation::resumeTimerFired)
145144
{
146145
}
@@ -731,6 +730,16 @@ void Geolocation::handlePendingPermissionNotifiers()
731730
}
732731
}
733732

733+
Navigator* Geolocation::navigator()
734+
{
735+
return m_navigator.get();
736+
}
737+
738+
Frame* Geolocation::frame() const
739+
{
740+
return m_navigator ? m_navigator->frame() : nullptr;
741+
}
742+
734743
} // namespace WebCore
735744

736745
#endif // ENABLE(GEOLOCATION)

‎Source/WebCore/Modules/geolocation/Geolocation.h‎

Lines changed: 13 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@ namespace WebCore {
4545
class Frame;
4646
class GeoNotifier;
4747
class GeolocationError;
48+
class Navigator;
4849
class Page;
4950
class ScriptExecutionContext;
5051
class SecurityOrigin;
@@ -54,12 +55,11 @@ class Geolocation final : public ScriptWrappable, public RefCounted<Geolocation>
5455
WTF_MAKE_ISO_ALLOCATED(Geolocation);
5556
friend class GeoNotifier;
5657
public:
57-
static Ref<Geolocation> create(ScriptExecutionContext*);
58+
static Ref<Geolocation> create(Navigator&);
5859
WEBCORE_EXPORT ~Geolocation();
5960

6061
WEBCORE_EXPORT void resetAllGeolocationPermission();
6162
Document* document() const { return downcast<Document>(scriptExecutionContext()); }
62-
Frame* frame() const { return document() ? document()->frame() : nullptr; }
6363

6464
void getCurrentPosition(Ref<PositionCallback>&&, RefPtr<PositionErrorCallback>&&, PositionOptions&&);
6565
int watchPosition(Ref<PositionCallback>&&, RefPtr<PositionErrorCallback>&&, PositionOptions&&);
@@ -73,8 +73,11 @@ class Geolocation final : public ScriptWrappable, public RefCounted<Geolocation>
7373
void setError(GeolocationError&);
7474
bool shouldBlockGeolocationRequests();
7575

76+
Navigator* navigator();
77+
WEBCORE_EXPORT Frame* frame() const;
78+
7679
private:
77-
explicit Geolocation(ScriptExecutionContext*);
80+
explicit Geolocation(Navigator&);
7881

7982
Geoposition* lastPosition();
8083

@@ -144,25 +147,20 @@ class Geolocation final : public ScriptWrappable, public RefCounted<Geolocation>
144147
bool haveSuitableCachedPosition(const PositionOptions&);
145148
void makeCachedPositionCallbacks();
146149

150+
void resumeTimerFired();
151+
152+
WeakPtr<Navigator> m_navigator;
147153
GeoNotifierSet m_oneShots;
148154
Watchers m_watchers;
149155
GeoNotifierSet m_pendingForPermissionNotifiers;
150156
RefPtr<Geoposition> m_lastPosition;
151157

152-
enum {
153-
Unknown,
154-
InProgress,
155-
Yes,
156-
No
157-
} m_allowGeolocation;
158-
bool m_isSuspended;
159-
bool m_resetOnResume;
160-
bool m_hasChangedPosition;
158+
enum { Unknown, InProgress, Yes, No } m_allowGeolocation { Unknown };
159+
bool m_isSuspended { false };
160+
bool m_resetOnResume { false };
161+
bool m_hasChangedPosition { false };
161162
RefPtr<PositionError> m_errorWaitingForResume;
162-
163-
void resumeTimerFired();
164163
Timer m_resumeTimer;
165-
166164
GeoNotifierSet m_requestsAwaitingCachedPosition;
167165
};
168166

‎Source/WebCore/Modules/geolocation/Geolocation.idl‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@
2727
[
2828
NoInterfaceObject,
2929
Conditional=GEOLOCATION,
30-
GenerateIsReachable=ImplFrame,
30+
GenerateIsReachable=ReachableFromNavigator,
3131
] interface Geolocation {
3232
// FIXME: PositionErrorCallback should not be nullable
3333
void getCurrentPosition(PositionCallback successCallback,

0 commit comments

Comments
 (0)