Bug 9902 – jsNull and NSNull not properly converted between JS and ObjC

• Component: WebCore JavaScript
• Status: RESOLVED WONTFIX

[Dan Waylonis]

Hi, I'm working for Google on a WebKit plugin.

I've found that JS code with a "null" value will be converted to [WebUndefined undefined] when sent to a WebKit plug-in, and that there is no way to return a JS "null" from the plug-in (returning nil results in a JS "undefined" value).

The documentation (http://developer.apple.com/documentation/Cocoa/Conceptual/DisplayWebContent/Tasks/JavaScriptFromObjC.html) states that NSNull will be converted to/from JS's null.

A very straightforward fix for this would be to add these lines:

objc_utility.mm:convertObjcValueToValue:(line 236)
                } else if ([*obj isKindOfClass:[NSNull class]]) {
                  aValue = jsNull();

WebScriptObject.mm:_convertValueToObjcValue:(line 436)
    else if (value->isNull())
        // JS Null -> NSNull
        return [NSNull null];

I came across this bug when I found that invokeUndefinedMethodFromWebScript:withArgs: was not being called for JS code that had a null in the arguments.  What happens is that inside of ObjcInstance::invokeMethod() the null is turned into a actual NULL (0x0) value, and when the code tries to add it to the objcArgs array, it throws an exception and the invocation never happens.

The above code changes fix the discrepancy between the documentation and reality as well as fixes the bug that I just mentioned.

Thanks,
Dan

[David Kilzer (:ddkilzer)]

Hi Dan, please post a patch for review.  You can use the WebKitTools/Scripts/svn-create-patch script create a patch for you to post to this bug:

$ ./WebKitTools/Scripts/svn-create-patch JavaScriptCore > bug-9902.diff

If you'd like to be really pro-active, create a ChangeLog entry before creating the patch above:

$ ./WebKitTools/Scripts/prepare-ChangeLog JavaScriptCore

You can learn more about coding styles and the process used to contribute code (patches) to WebKit on this web page:

http://webkit.opendarwin.org/coding/contributing.html

Thanks!

[Dan Waylonis]

Created attachment 9451 [details]
Patch to fix JSnull <-> NSNull

[Darin Adler]

Comment on attachment 9451 [details]
Patch to fix JSnull <-> NSNull

Patch looks great.

We don't want that "GOOGLE Change" comment in there, though. And the closing parenthesis is missing from this line:

+    // Other types (UnspecifiedType converted to 0.

Ideally we also need to come up with a test for this.

[Dan Waylonis]

Created attachment 9530 [details]
Revised changes for 9902

Changes as suggested by Darin

[Darin Adler]

Comment on attachment 9530 [details]
Revised changes for 9902

Patch looks great! All we need is a layout test, and a change log entry, and we're ready to go.

[David Kilzer (:ddkilzer)]

(In reply to comment #5)
> Patch looks great! All we need is a layout test, and a change log entry, and
> we're ready to go.

Because this bug deals with the ObjC<->JavaScript bindings, a normal layout test (which tests the behavior of the browser using "normal" HTML and JavaScript) is not possible.

However, I think there is a (relatively) new feature of DumpRenderTree (the testing harness application WebKit uses to run layout tests) that allows you to call ObjC methods from within JavaScript.  I'm not real familiar with how to do this, though.  I would start by looking in the LayoutTests/plugins/ directory for any test with "objc" in the name.

[Dan Waylonis]

Created attachment 9533 [details]
Changelog patch

[Darin Adler]

Comment on attachment 9533 [details]
Changelog patch

Please don't use a separate patch for ChangeLog. We use single patches that include ChangeLog, code changes, and test cases.

Also, this has tab characters in it. We don't use tab characters, and we have Subversion configured so it won't allow check-ins of files with tab characters in them.

Also, it's best if the change has comments explaining the individual changes in addition to the bug URL, although some are better than others at adhering to this.

[Dan Waylonis]

Created attachment 9535 [details]
Revised changes for 9902

I've removed the tabs and added comments to the change long.  I also modified the DumpRenderTree code to include a method called "echo" which will take an object and return it.  I modified LayoutTests/plugins/bindings-test.html and LayoutTests/plugins/bindings-test-expected.txt to test that objects are properly round-tripped between JS and ObjC.

[Darin Adler]

Wow, I'm really curious about how this bug affects Google!

[David Kilzer (:ddkilzer)]

Comment on attachment 9535 [details]
Revised changes for 9902

I think Attachment 9535 [details] needed its review flag set.

[Darin Adler]

Comment on attachment 9535 [details]
Revised changes for 9902

r=me

ChangeLog needs a tiny bit of work. Whoever lands this should take care of that.

[Alexey Proskuryakov]

I attempted to land, but this patch has already bit-rotted (WebScriptObject.mm moved to WebCore, and WebScriptObject.h now has a split personality).

We maintain a separate ChangeLog in each subproject, so a patch should ideally provide them all.

[Darin Adler]

I fixed up this patch and landed it.

Committed revision 15618.

[Timothy Hatcher]

This patch caused a couple of regressions. It seems application and widget authors do not take into acount for NSNull being a return type, this is very unfortunate.

<rdar://problem/4651318> <rdar://problem/4701626>

One example of this in Dashboard causes the following exception.

2006-08-29 17:09:05.927 DashboardClient[24975] *** -[NSNull length]: selector not recognized [self = 0xa0801040]

[Timothy Hatcher]

Should we roll this out?

[Darin Adler]

(In reply to comment #16)
> Should we roll this out?

We probably need to -- I think we're stuck with both null and undefined turning into nil. We should correct the documentation instead.

[Dan Waylonis]

That's super unfortunate.  It will definitely cause my project here to fail.

How about adding a class method +(void)setUsesNSNullForNil:(BOOL)yn to the WebScriptObject class?  The default behavior is the old behavior.  If set to YES, then it functions as the documentation describes and the patch fixes.

[Darin Adler]

(In reply to comment #18)
> How about adding a class method +(void)setUsesNSNullForNil:(BOOL)yn to the
> WebScriptObject class?  The default behavior is the old behavior.  If set to
> YES, then it functions as the documentation describes and the patch fixes.

I don't think that will work. That would change behavior for everyone in the same process, which could include plug-ins and other clients of WebKit.

[Dan Waylonis]

That's true, but it wouldn't break existing Dashboard widgets, which I think were the bugs reported. 

Is there some way to make it specific to a particular instance of a plugin?  Perhaps if the plugin responded to a selector +(BOOL)useNSNullForNil;?

[David Kilzer (:ddkilzer)]

(In reply to comment #19)
> (In reply to comment #18)
> > How about adding a class method +(void)setUsesNSNullForNil:(BOOL)yn to the
> > WebScriptObject class?  The default behavior is the old behavior.  If set to
> > YES, then it functions as the documentation describes and the patch fixes.
> 
> I don't think that will work. That would change behavior for everyone in the
> same process, which could include plug-ins and other clients of WebKit.

What about adding a new public API method that does the right thing and deprecating (and documenting) the old method?  The implemention of the two methods could be refactored to remove duplicate code while maintaining both old and new behaviors.

Reopening bug since it's apparent changes need to be made.

[Darin Adler]

(In reply to comment #20)
> That's true, but it wouldn't break existing Dashboard widgets, which I think
> were the bugs reported. 

It easily could break an existing Dashboard widget. Upcoming versions of Dashboard load multiple widgets in a single process, and if a widget loaded a plug-in that made this call, then it could break plug-ins in other widgets.

We don't know that this issue is limited to Dashboard. It's true that the first trouble we found was in a Dashboard widget.

> Is there some way to make it specific to a particular instance of a plugin? 
> Perhaps if the plugin responded to a selector +(BOOL)useNSNullForNil;?

Could be -- I'm not sure how to get to the plug-in from inside the bindings code efficiently, but perhaps it could be done. Sounds challenging to me.

[David Kilzer (:ddkilzer)]

Comment on attachment 9535 [details]
Revised changes for 9902

Clearing review+ flag on Attachment 9535 [details] after bug was reopened.  See Comment #21.

[Darin Adler]

(In reply to comment #21)
> What about adding a new public API method that does the right thing and
> deprecating (and documenting) the old method?  The implemention of the two
> methods could be refactored to remove duplicate code while maintaining both old
> and new behaviors.

Might be a good approach. Lets figure out what methods are affected.

[Dan Waylonis]

I see that in the latest top of tree the WebScriptObject.mm:_convertValueToObjCValue:originExecutionContext:executionContext: method now no longer returns NSNull when converting a jsNull for compatibility reasons.

It would be great if there was a way that the Plugin could indicate that it wants to receive NSNulls instead of 0 for jsNull conversion.  Maybe some way of querying the plugin once and setting the Exec state to control the conversion? 

[Darin Adler]

(In reply to comment #25)
> It would be great if there was a way that the Plugin could indicate that it
> wants to receive NSNulls instead of 0 for jsNull conversion.  Maybe some way of
> querying the plugin once and setting the Exec state to control the conversion? 

That sounds OK, or the thing mentioned in the thread leading to comment #24.

[Anders Carlsson]

We don't plan to modify the Objective-C bindings further.