Modify toNative functions in JSDOMBinding.h to take an ExecState reference instead of pointer.
Created attachment 282435 [details] Patch
Created attachment 282438 [details] Patch
This is a preliminary patch for Bug 158835 (c.f. Comment #10).
Comment on attachment 282438 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=282438&action=review > Source/WebCore/bindings/js/JSBlobCustom.cpp:75 > - JSObject* blobParts = toJSSequence(exec, exec->argument(0), blobPartsLength); > + JSObject* blobParts = toJSSequence(*exec, exec->argument(0), blobPartsLength); There's no null check for exec in this method, so this seems bogus. > Source/WebCore/bindings/js/JSDOMBinding.h:480 > -inline JSC::JSObject* toJSSequence(JSC::ExecState* exec, JSC::JSValue value, unsigned& length) > +inline JSC::JSObject* toJSSequence(JSC::ExecState& state, JSC::JSValue value, unsigned& length) I'm not a fan of the rename. We historically use "exec", "execState" and "state" for execState variables. While we would ideally choose one and stick with it, I think "state" is the worse because it's the most generic and vague. Please leave it as "exec" or change to "execState" if you don't like exec for some reason. > Source/WebCore/bindings/js/JSDOMBinding.h:663 > - static inline bool nativeValue(JSC::ExecState* exec, JSC::JSValue jsValue, String& indexedValue) > + static inline bool nativeValue(JSC::ExecState& state, JSC::JSValue jsValue, String& indexedValue) Same comment on the variable rename here, and everywhere else you might do it. > Source/WebCore/bindings/js/JSDictionary.cpp:156 > - JSObject* object = toJSSequence(exec, value, length); > + JSObject* object = toJSSequence(*exec, value, length); There's no null check for exec in this method, so this seems bogus. > Source/WebCore/bindings/js/JSDictionary.cpp:219 > - JSObject* object = toJSSequence(exec, value, length); > + JSObject* object = toJSSequence(*exec, value, length); There's no null check for exec in this method, so this seems bogus. > Source/WebCore/bindings/js/JSDictionary.cpp:287 > - JSObject* object = toJSSequence(exec, value, length); > + JSObject* object = toJSSequence(*exec, value, length); There's no null check for exec in this method, so this seems bogus. > Source/WebCore/bindings/js/JSFileCustom.cpp:55 > - JSObject* blobParts = toJSSequence(exec, arg, blobPartsLength); > + JSObject* blobParts = toJSSequence(*exec, arg, blobPartsLength); There's no null check for exec in this method, so this seems bogus. Let's pretend I make the same comment everywhere you dereference exec and the method does't have a prior null check. > Source/WebCore/bindings/scripts/test/JS/JSTestTypedefs.cpp:458 > - auto x = toNativeArray<int32_t>(state, state->argument(0)); > + auto x = toNativeArray<int32_t>(*state, state->argument(0)); And I meant that everywhere, including in "test only" code.
Note that an eventual goal is that we're never passing ExecState*'s around in bindings code at all. This is an intermediate step along that path. The null checks will eventually be unnecessary.
(In reply to comment #4) > Comment on attachment 282438 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=282438&action=review > > > Source/WebCore/bindings/js/JSBlobCustom.cpp:75 > > - JSObject* blobParts = toJSSequence(exec, exec->argument(0), blobPartsLength); > > + JSObject* blobParts = toJSSequence(*exec, exec->argument(0), blobPartsLength); > > There's no null check for exec in this method, so this seems bogus. constructJSBlob should actually take an ExecState reference not a pointer. Migrating to ExecState references needs to start somewhere so this patch seems to go in the right direction to me. As of the null check, I guess an ASSERT(exec) at the beginning of the function would be good enough.
(In reply to comment #6) > (In reply to comment #4) > > Comment on attachment 282438 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=282438&action=review > > > > > Source/WebCore/bindings/js/JSBlobCustom.cpp:75 > > > - JSObject* blobParts = toJSSequence(exec, exec->argument(0), blobPartsLength); > > > + JSObject* blobParts = toJSSequence(*exec, exec->argument(0), blobPartsLength); > > > > There's no null check for exec in this method, so this seems bogus. > > constructJSBlob should actually take an ExecState reference not a pointer. > Migrating to ExecState references needs to start somewhere That was the crux of https://bugs.webkit.org/show_bug.cgi?id=159298#c5 > so this patch seems to go in the right direction to me. Sure, me too. > As of the null check, I guess an ASSERT(exec) at the beginning of the > function would be good enough. Let's pretend that Nael lands this patch, then immediately no longer has time to work on this task, and nobody else has time either. So we're left with this patch in the tree for some time. In that scenario, I'd rather see a proper null check along with signaling an error condition, wherever that is possible. (There's at least a few places where it is.) Elsewhere, an ASSERT would be fine.
> > constructJSBlob should actually take an ExecState reference not a pointer. > > Migrating to ExecState references needs to start somewhere > > That was the crux of https://bugs.webkit.org/show_bug.cgi?id=159298#c5 FWIW, I used ASSERT() for JS builtin constructors in JSDOMConstructor.h IIRC, passing an ExecState& to constructors requires changes in JSC. Maybe not complex but probably quite some edit changes to be done in one step. A good step forward for WebCore might be to update the binding generator to pass an ExecState& for custom constructors. The binding generator would actually generate the ASSERT() just before calling the constructXX function. Nael, wdyt?
Created attachment 282559 [details] Patch
Thanks for the reviews. Since the final goal is to pass ExecState reference in bindings code, using ASSERT() as interim solution seems good to me. In the uploaded patch, I added assertions to check exec is not null when it is dereferenced. To avoid a lot of modifications in this patch, I added a follow-up bug for the update of JS binding code generator for Custom Constructor (Bug 159357). I will update this patch once Bug 159357 is resolved.
Created attachment 283317 [details] Patch
Bug 159357 is now resolved: ExecState is passed by reference to custom constructors and so null checks are not needed anymore. ASSERT() is added in JSDictionary::convertValue() methods which use toJSequence() to check exec is not null.
Comment on attachment 283317 [details] Patch Clearing flags on attachment: 283317 Committed r203065: <http://trac.webkit.org/changeset/203065>
All reviewed patches have been landed. Closing bug.