Bug 204879 – Do not use DOMGuarded for maplike

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[youenn fablet]

Do not use DOMGuarded for maplike.
This is not really useful right now, instead we can reuse setlike model.

[youenn fablet]

Created attachment 384887 [details]
Patch

[youenn fablet]

Created attachment 384992 [details]
Patch

[youenn fablet]

Created attachment 384995 [details]
Patch

[Darin Adler]

Comment on attachment 384995 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        Update maplike according setlike model where we support syncing at creation of the JS wrapper
> +        and syncing whenever setlike/maplike methods are called.

I don’t fully understand the model here.

This assumes all calls to change the state of a maplike or setlike are made from DOM bindings. If code other than the DOM bindings modifies such an object’s state directly, then the wrapper will be out of sync? If that’s so, then it’s dangerous to have many functions that could be called from elsewhere in WebKit, but it’s unsafe to call them.

Can we do better? I don’t mind the concept here, but I’m worried that this is possibly easy to use wrong.

[youenn fablet]

The current use of setlike/maplike is limited to adding items at init time of the JS wrapper and updating through DOM binding. Hence the proposed model.

When we will want to support updating from C++ and then the JS wrapper backing set/map, I see two options:
1. Everytime a setlike/maplike method is called from binding generated code, we first call a synchronise method to sync the backing set/map (brute force or optimised).
2. At the time we update the C++ map/set, we create the JS wrapper (if it exists, otherwise we can return early) and update its backing set/map. We anyway need to call toJS on the key/value to update the backing set/map so getting the JS wrapper should be feasible.

[Darin Adler]

(In reply to youenn fablet from comment #5)
> The current use of setlike/maplike is limited to adding items at init time
> of the JS wrapper and updating through DOM binding. Hence the proposed model.

Sure, got it.

As long as this is true, I kind of wish the functions that modify the object were not accessible to other WebKit C++ code that’s not generated by the bindings script. That way you can’t make the mistake of calling them. So many other DOM functions are both accessible from the DOM bindings, but also functions that are safe to call from elsewhere in WebKit, and I can imagine someone not understanding that is true here.

Something simple like making the functions private and using friend, giving the functions strange names, or giving the functions unusual calling conventions. None of those options seem excellent to me, but maybe there’s a good/simple idea that could achieve this.

[Darin Adler]

Comment on attachment 384995 [details]
Patch

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

> Source/WebCore/bindings/js/JSDOMMapLike.cpp:39
> +    if (backingMap.isUndefined()) {

I would like this better with an early exit:

    if (!undefined)
        // less code

> Source/WebCore/bindings/js/JSDOMMapLike.cpp:52
> +void clearBackingMap(JSC::JSGlobalObject& lexicalGlobalObject, JSC::JSObject& backingMap)

Could we refactor this so clearBackingMap and setBackingMap share more code? I think we could make a helper function that takes a property name and a argument buffer and does all the work.

> Source/WebCore/bindings/js/JSDOMMapLike.cpp:56
> +    ASSERT(!function.isUndefined());

What makes this a safe assertion? I don’t see how we can guarantee this. But also, I don’t think we need to assert it since getCallData can handle undefined.

> Source/WebCore/bindings/js/JSDOMMapLike.cpp:60
> +    ASSERT(callType != JSC::CallType::None);

What makes this a safe assertion? I don’t see how we can guarantee this.

> Source/WebCore/bindings/js/JSDOMMapLike.cpp:69
> +    ASSERT(!function.isUndefined());

What makes this a safe assertion? I don’t see how we can guarantee this. But also, I don’t think we need to assert it since getCallData can handle undefined.

> Source/WebCore/bindings/js/JSDOMMapLike.cpp:73
> +    ASSERT(callType != JSC::CallType::None);

What makes this a safe assertion? I don’t see how we can guarantee this.

> Source/WebCore/bindings/js/JSDOMMapLike.h:56
> +    template<class IDLKeyType, class IDLValueType> void set(typename IDLKeyType::ParameterType, typename IDLValueType::ParameterType);

I personally prefer typename to class for the template arguments.

> Source/WebCore/bindings/js/JSDOMMapLike.h:70
> +template<class IDLKeyType, class IDLValueType>

Ditto.

> Source/WebCore/bindings/js/JSDOMMapLike.h:71
> +inline void DOMMapAdapter::set(typename IDLKeyType::ParameterType key, typename IDLValueType::ParameterType value)

I don’t think we need inline here. It probably does not affect whether this is inlined or not, and outside of that the main function of "inline" is to make it safe to have something in the header, which is already the case for a function template.

> Source/WebCore/bindings/js/JSDOMMapLike.h:79
> +inline void DOMMapAdapter::clear()

Why should this be inline and in the header?

> Source/WebCore/bindings/js/JSDOMMapLike.h:84
> +template<typename WrapperClass> inline JSC::JSObject& getAndInitializeBackingMap(JSC::JSGlobalObject& lexicalGlobalObject, WrapperClass& mapLike)

Don’t think we need inline here (same reason as above>.

> Source/WebCore/bindings/js/JSDOMMapLike.h:94
>  template<typename WrapperClass> inline JSC::JSValue forwardSizeToMapLike(JSC::JSGlobalObject& lexicalGlobalObject, WrapperClass& mapLike)

Don’t think we need inline here (same reason as above>. Same for the other functions.

[youenn fablet]

Created attachment 385121 [details]
Patch for landing

[youenn fablet]

Thanks for the review, I updated accordingly.

[WebKit Commit Bot]

Comment on attachment 385121 [details]
Patch for landing

Clearing flags on attachment: 385121

Committed r253276: <https://trac.webkit.org/changeset/253276>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/57735209>

[Truitt Savell]

It looks like the changes in https://trac.webkit.org/changeset/253276/webkit

broke webrtc/multi-video.html 

tracking in https://bugs.webkit.org/show_bug.cgi?id=205020

[youenn fablet]

(In reply to Truitt Savell from comment #13)
> It looks like the changes in https://trac.webkit.org/changeset/253276/webkit
> 
> broke webrtc/multi-video.html 
> 
> tracking in https://bugs.webkit.org/show_bug.cgi?id=205020

I doubt this change set caused the regression.
I'll look at it tomorrow.