Bug 9902 - jsNull and NSNull not properly converted between JS and ObjC
Summary: jsNull and NSNull not properly converted between JS and ObjC
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2006-07-13 18:10 PDT by Dan Waylonis
Modified: 2016-08-04 15:14 PDT (History)
7 users (show)

See Also:


Attachments
Patch to fix JSnull <-> NSNull (1.75 KB, patch)
2006-07-14 12:04 PDT, Dan Waylonis
no flags Details | Formatted Diff | Diff
Revised changes for 9902 (2.62 KB, patch)
2006-07-17 14:59 PDT, Dan Waylonis
darin: review-
Details | Formatted Diff | Diff
Changelog patch (734 bytes, patch)
2006-07-17 16:09 PDT, Dan Waylonis
no flags Details | Formatted Diff | Diff
Revised changes for 9902 (4.85 KB, patch)
2006-07-17 17:11 PDT, Dan Waylonis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dan Waylonis 2006-07-13 18:10:36 PDT
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
Comment 1 David Kilzer (:ddkilzer) 2006-07-13 21:18:20 PDT
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!

Comment 2 Dan Waylonis 2006-07-14 12:04:45 PDT
Created attachment 9451 [details]
Patch to fix JSnull <-> NSNull
Comment 3 Darin Adler 2006-07-14 19:43:23 PDT
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.
Comment 4 Dan Waylonis 2006-07-17 14:59:36 PDT
Created attachment 9530 [details]
Revised changes for 9902

Changes as suggested by Darin
Comment 5 Darin Adler 2006-07-17 15:45:18 PDT
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.
Comment 6 David Kilzer (:ddkilzer) 2006-07-17 15:58:32 PDT
(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.
Comment 7 Dan Waylonis 2006-07-17 16:09:53 PDT
Created attachment 9533 [details]
Changelog patch
Comment 8 Darin Adler 2006-07-17 16:51:31 PDT
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.
Comment 9 Dan Waylonis 2006-07-17 17:11:22 PDT
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.
Comment 10 Darin Adler 2006-07-18 14:05:47 PDT
Wow, I'm really curious about how this bug affects Google!
Comment 11 David Kilzer (:ddkilzer) 2006-07-20 07:13:38 PDT
Comment on attachment 9535 [details]
Revised changes for 9902

I think Attachment 9535 [details] needed its review flag set.
Comment 12 Darin Adler 2006-07-20 09:35:34 PDT
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.
Comment 13 Alexey Proskuryakov 2006-07-24 09:56:04 PDT
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.
Comment 14 Darin Adler 2006-07-24 21:59:08 PDT
I fixed up this patch and landed it.

Committed revision 15618.
Comment 15 Timothy Hatcher 2006-08-30 10:12:02 PDT
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]

Comment 16 Timothy Hatcher 2006-08-30 10:12:45 PDT
Should we roll this out?
Comment 17 Darin Adler 2006-08-30 10:17:47 PDT
(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.
Comment 18 Dan Waylonis 2006-08-30 11:40:31 PDT
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.
Comment 19 Darin Adler 2006-08-30 12:51:56 PDT
(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.
Comment 20 Dan Waylonis 2006-08-30 14:47:58 PDT
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;?
Comment 21 David Kilzer (:ddkilzer) 2006-08-30 15:05:48 PDT
(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.
Comment 22 Darin Adler 2006-08-30 15:06:22 PDT
(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.
Comment 23 David Kilzer (:ddkilzer) 2006-08-30 15:06:39 PDT
Comment on attachment 9535 [details]
Revised changes for 9902

Clearing review+ flag on Attachment 9535 [details] after bug was reopened.  See Comment #21.
Comment 24 Darin Adler 2006-08-30 15:07:23 PDT
(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.
Comment 25 Dan Waylonis 2006-09-26 18:08:09 PDT
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? 
Comment 26 Darin Adler 2006-09-27 10:20:43 PDT
(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.
Comment 27 Anders Carlsson 2016-08-04 15:14:03 PDT
We don't plan to modify the Objective-C bindings further.