Bug 200436 – navigator.geolocation wrapper should not become GC-collectable once its frame is detached

• Component: Bindings
• Status: RESOLVED FIXED

[Chris Dumez]

navigator.geolocation wrapper should not become GC-collectable once its frame is detached, given that it can outlive the frame.

[Chris Dumez]

Created attachment 375530 [details]
Patch

[EWS Watchlist]

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.

[Chris Dumez]

Created attachment 375531 [details]
Patch

[EWS Watchlist]

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.

[Chris Dumez]

Created attachment 375533 [details]
Patch

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[Chris Dumez]

Created attachment 375542 [details]
Patch

[Chris Dumez]

Created attachment 375543 [details]
Patch

[EWS Watchlist]

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.

[Darin Adler]

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.

[Darin Adler]

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.

[Chris Dumez]

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.

[Chris Dumez]

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;
    }

[Chris Dumez]

Created attachment 375556 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 375556 [details]
Patch

Clearing flags on attachment: 375556

Committed r248276: <https://trac.webkit.org/changeset/248276>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/53962896>

[Alexey Proskuryakov]

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?

[Chris Dumez]

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

[Chris Dumez]

<https://trac.webkit.org/changeset/248306>

[Darin Adler]

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