navigator.geolocation wrapper should not become GC-collectable once its frame is detached, given that it can outlive the frame.
Created attachment 375530 [details] Patch
Attachment 375530 [details] did not pass style-queue: ERROR: Source/WebCore/Modules/geolocation/Geolocation.h:164: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/Modules/geolocation/Geolocation.h:165: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/Modules/geolocation/Geolocation.h:166: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] Total errors found: 3 in 23 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 375531 [details] Patch
Attachment 375531 [details] did not pass style-queue: ERROR: Source/WebCore/Modules/geolocation/Geolocation.h:164: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/Modules/geolocation/Geolocation.h:165: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/Modules/geolocation/Geolocation.h:166: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] Total errors found: 3 in 23 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 375533 [details] Patch
Attachment 375533 [details] did not pass style-queue: ERROR: Source/WebCore/Modules/geolocation/Geolocation.h:164: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/Modules/geolocation/Geolocation.h:165: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/Modules/geolocation/Geolocation.h:166: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] Total errors found: 3 in 24 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 375533 [details] Patch Attachment 375533 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/12864090 New failing tests: fast/dom/navigator-property-gc-after-frame-detach.html
Created attachment 375537 [details] Archive of layout-test-results from ews103 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-highsierra Platform: Mac OS X 10.13.6
Created attachment 375542 [details] Patch
Created attachment 375543 [details] Patch
Attachment 375543 [details] did not pass style-queue: ERROR: Source/WebCore/Modules/geolocation/Geolocation.h:164: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/Modules/geolocation/Geolocation.h:165: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebCore/Modules/geolocation/Geolocation.h:166: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] Total errors found: 3 in 25 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 375543 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375543&action=review Looks great. > Source/WebCore/Modules/geolocation/Geolocation.h:77 > + WEBCORE_EXPORT Frame* frame() const; I guess there must be important clients of this function, but seems annoying that we have it. > Source/WebCore/Modules/geolocation/Geolocation.h:163 > + } m_allowGeolocation { Unknown }; Not sure you will agree, but I think this would read slightly better all on one line: enum { Unknown, InProgress, Yes, No } m_allowGeolocation { Unknown }; > Source/WebCore/Modules/geolocation/NavigatorGeolocation.cpp:76 > + m_geolocation = Geolocation::create(*m_navigator); Is this correct for the case where m_navigator is nullptr? I assume it can be null because it’s a WeakPtr. > Source/WebCore/bindings/js/JSNavigatorCustom.cpp:33 > + visitor.addOpaqueRoot(static_cast<NavigatorBase*>(&wrapped())); I wish there was a safer cast for upcasts rather than static_cast, which can also do other kinds of casting like downcasts. I also wish there was less void* danger here. If we passed &wrapped() without casting to NavigatorBase*, how would we know we had it wrong? > Source/WebCore/bindings/js/JSWorkerNavigatorCustom.cpp:33 > + visitor.addOpaqueRoot(static_cast<NavigatorBase*>(&wrapped())); Ditto. > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4686 > + $rootString = " auto* root = static_cast<NavigatorBase*>(WTF::getPtr(js${interfaceName}->wrapped().navigator()));\n"; Ditto. > Source/WebCore/plugins/DOMMimeTypeArray.h:43 > + Navigator* navigator() { return m_navigator.get(); } Inconsistent that the other getter functions are const, but not this. > Source/WebCore/plugins/DOMMimeTypeArray.h:44 > + Frame* frame() const { return m_navigator ? m_navigator->frame() : nullptr; } I assume we need these because there was a DOMWindowProperty::frame function. But since it’s not new I can’t see the call sites in this patch. Seems like it would be more elegant if we didn’t need these, but maybe if I saw the call sites I would understand why we do. > Source/WebCore/plugins/DOMPluginArray.h:47 > + Frame* frame() const { return m_navigator ? m_navigator->frame() : nullptr; } Ditto.
Comment on attachment 375543 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375543&action=review >> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4686 >> + $rootString = " auto* root = static_cast<NavigatorBase*>(WTF::getPtr(js${interfaceName}->wrapped().navigator()));\n"; > > Ditto. Since it’s a local variable we could write this without the static_cast: NavigatorBase* root = WTF::getPtr(js${interfaceName}->wrapped().navigator()); But not sure if overall that makes this better or worse.
Comment on attachment 375543 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375543&action=review >> Source/WebCore/Modules/geolocation/Geolocation.h:77 >> + WEBCORE_EXPORT Frame* frame() const; > > I guess there must be important clients of this function, but seems annoying that we have it. Yes, this one is actually used by WebKit2 or WebKitLegacy, which is why I had to export it. I will check what it is used for. >> Source/WebCore/Modules/geolocation/Geolocation.h:163 >> + } m_allowGeolocation { Unknown }; > > Not sure you will agree, but I think this would read slightly better all on one line: > > enum { Unknown, InProgress, Yes, No } m_allowGeolocation { Unknown }; Ok. >> Source/WebCore/Modules/geolocation/NavigatorGeolocation.cpp:76 >> + m_geolocation = Geolocation::create(*m_navigator); > > Is this correct for the case where m_navigator is nullptr? I assume it can be null because it’s a WeakPtr. It actually cannot be null, this is a NavigatorSupplement so it is owned by Navigator. I should not be using a WeakPtr in this class. >> Source/WebCore/bindings/js/JSNavigatorCustom.cpp:33 >> + visitor.addOpaqueRoot(static_cast<NavigatorBase*>(&wrapped())); > > I wish there was a safer cast for upcasts rather than static_cast, which can also do other kinds of casting like downcasts. I also wish there was less void* danger here. If we passed &wrapped() without casting to NavigatorBase*, how would we know we had it wrong? It is a bit error-prone yes, but my test would fail if we would get it wrong. >>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4686 >>> + $rootString = " auto* root = static_cast<NavigatorBase*>(WTF::getPtr(js${interfaceName}->wrapped().navigator()));\n"; >> >> Ditto. > > Since it’s a local variable we could write this without the static_cast: > > NavigatorBase* root = WTF::getPtr(js${interfaceName}->wrapped().navigator()); > > But not sure if overall that makes this better or worse. I like it, will switch. >> Source/WebCore/plugins/DOMMimeTypeArray.h:44 >> + Frame* frame() const { return m_navigator ? m_navigator->frame() : nullptr; } > > I assume we need these because there was a DOMWindowProperty::frame function. But since it’s not new I can’t see the call sites in this patch. Seems like it would be more elegant if we didn’t need these, but maybe if I saw the call sites I would understand why we do. Those frame() getters on DOMMimeTypeArray and DOMPluginArray are only used by the internal implementation iirc (for early returns). At the very least, I can likely make them private. >> Source/WebCore/plugins/DOMPluginArray.h:47 >> + Frame* frame() const { return m_navigator ? m_navigator->frame() : nullptr; } > > Ditto. See comment above.
Comment on attachment 375543 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375543&action=review >>> Source/WebCore/Modules/geolocation/Geolocation.h:77 >>> + WEBCORE_EXPORT Frame* frame() const; >> >> I guess there must be important clients of this function, but seems annoying that we have it. > > Yes, this one is actually used by WebKit2 or WebKitLegacy, which is why I had to export it. I will check what it is used for. /Volumes/Data/WebKit/OpenSource/Source/WebKitLegacy/mac/WebCoreSupport/WebGeolocationClient.mm:114:32: error: 'frame' is a private member of 'WebCore::Geolocation' Frame *frame = geolocation.frame(); if (!frame) { geolocation.setIsAllowed(false); return; }
Created attachment 375556 [details] Patch
Comment on attachment 375556 [details] Patch Clearing flags on attachment: 375556 Committed r248276: <https://trac.webkit.org/changeset/248276>
All reviewed patches have been landed. Closing bug.
<rdar://problem/53962896>
Comment on attachment 375556 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375556&action=review > LayoutTests/platform/wk2/TestExpectations:-163 > -webkit.org/b/141122 editing/selection/programmatic-selection-on-mac-is-directionless.html [ Pass Failure ] This change landed accidentally, could you please undo it?
(In reply to Alexey Proskuryakov from comment #20) > Comment on attachment 375556 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=375556&action=review > > > LayoutTests/platform/wk2/TestExpectations:-163 > > -webkit.org/b/141122 editing/selection/programmatic-selection-on-mac-is-directionless.html [ Pass Failure ] > > This change landed accidentally, could you please undo it? Ah.. That will teach me to work on 2 things at the same time. Will fix now.
<https://trac.webkit.org/changeset/248306>
(In reply to Chris Dumez from comment #15) > /Volumes/Data/WebKit/OpenSource/Source/WebKitLegacy/mac/WebCoreSupport/ > WebGeolocationClient.mm:114:32: error: 'frame' is a private member of > 'WebCore::Geolocation' So we need to keep this as long as we want to support the webView:decidePolicyForGeolocationRequestFromOrigin:frame:listener: delegate method on the iOS platform family. As an aside, I think we could consider dropping support for that at some point as part of cutting down the complexity of WebKitLegacy.