Bug 54898 – InjectedBundleNodeHandle dies too early in WKBundleHitTestResultGetNodeHandle

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[Alice Liu]

When asking a WKBundleHitTestResultRef for its nodeHandle, the InjectedBundleNodeHandle dies too early in WKBundleHitTestResultGetNodeHandle.

 	WebKit.dll!WebKit::InjectedBundleNodeHandle::~InjectedBundleNodeHandle()  Line 90	C++
 	WebKit.dll!WebKit::InjectedBundleNodeHandle::`scalar deleting destructor'()  + 0x16 bytes	C++
 	WebKit.dll!WTF::RefCounted<WebKit::APIObject>::deref()  Line 141 + 0x3a bytes	C++
 	WebKit.dll!WTF::derefIfNotNull<WebKit::InjectedBundleNodeHandle>(WebKit::InjectedBundleNodeHandle * ptr=0x03194368)  Line 60	C++
 	WebKit.dll!WTF::RefPtr<WebKit::InjectedBundleNodeHandle>::~RefPtr<WebKit::InjectedBundleNodeHandle>()  Line 58 + 0x19 bytes	C++
>	WebKit.dll!WKBundleHitTestResultGetNodeHandle(const OpaqueWKBundleHitTestResult * hitTestResultRef=0x0323a848)  Line 44 + 0x1c bytes	C++

[Alice Liu]

Created attachment 83200 [details]
patch

I don't know if this is right... the other functions that use release().leakRef() or release.releaseRef() that return WKBundleNodeHandleRef seem to have "copy" in their function names, but not WKBundleHitTestResultGetNodeHandle.

[Adam Roben (:aroben)]

Comment on attachment 83200 [details]
patch

My guess is that you're fixing a crash. It would be good to have "crash" somewhere in the bug title (which would also get it into your ChangeLog).

As you suspected, this change makes WKBundleHitTestResultGetNodeHandle follow the Create or Copy pattern, so its name will have to be changed.

The actual code change seems fine. Is it possible to make a test for this? I'd imagine it is, by using TestWebKitAPI.

[Alice Liu]

Created attachment 83402 [details]
API test

[WebKit Review Bot]

Attachment 83402 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/TestWebKitAPI/In..." exit_code: 1

Tools/TestWebKitAPI/Tests/WebKit2/HitTestResultNodeHandle.cpp:47:  Missing space after ,  [whitespace/comma] [3]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Roben (:aroben)]

Comment on attachment 83402 [details]
API test

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

Awesome that you got a test working!

> Tools/TestWebKitAPI/PlatformWebView.h:65
> +    void simulateRightClick(CGPoint pt);

No need for "pt" here. (If you did need an argument name, "point" would be better.) Maybe it should be a const CGPoint&?

It's slightly unfortunate to use CG types in cross-platform code, as this will make it harder for other ports to build TestWebKitAPI. Maybe two unsigneds would be better.

> Tools/TestWebKitAPI/Tests/WebKit2/HitTestResultNodeHandle.cpp:70
> +    WKContextInjectedBundleClient injectedBundleClient;
> +    memset(&injectedBundleClient, 0, sizeof(injectedBundleClient));
> +    injectedBundleClient.version = 0;
> +    injectedBundleClient.clientInfo = 0;
> +    injectedBundleClient.didReceiveMessageFromInjectedBundle = didReceiveMessageFromInjectedBundle;
> +    WKContextSetInjectedBundleClient(context.get(), &injectedBundleClient);

A separate setInjectedBundleClient function would make the main test logic easier to follow.

> Tools/TestWebKitAPI/Tests/WebKit2/HitTestResultNodeHandle.cpp:82
> +    TEST_ASSERT(done);

No need for this assertion, as it will never be reached if done is still false.

> Tools/TestWebKitAPI/Tests/WebKit2/HitTestResultNodeHandle_Bundle.cpp:49
> +        WKRetainPtr<WKStringRef> doneMessageName(AdoptWK, WKStringCreateWithUTF8CString("HitTestResultNodeHandleTestDoneMessageName"));
> +        WKRetainPtr<WKStringRef> doneMessageBody(AdoptWK, WKStringCreateWithUTF8CString("HitTestResultNodeHandleTestDoneMessageBody"));

You can use the Util::toWK function here instead.

> Tools/TestWebKitAPI/win/PlatformWebViewWin.cpp:109
> +    ::SendMessage(window, WM_RBUTTONDOWN, 0, MAKELPARAM(pt.x, pt.y));
> +    ::SendMessage(window, WM_RBUTTONUP, 0, MAKELPARAM(pt.x, pt.y));

Please use ::SendMessageW.

[Adam Roben (:aroben)]

Comment on attachment 83402 [details]
API test

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

> Tools/TestWebKitAPI/Tests/WebKit2/HitTestResultNodeHandle_Bundle.cpp:44
> +        WKBundleNodeHandleRef nodeHandle = WKBundleHitTestResultGetNodeHandle(hitTestResult);

Presumably you're leaking this nodeHandle, now that you've changed the API to a Copy-style API.

[Alice Liu]

Created attachment 84163 [details]
all-in-one patch

Made a tweak to the test.  changed to use a WKRetainPtr so that the object that is returned from the (fixed) API call is ref'ed, so that it will crash in builds where the API call is not fixed.  

also renamed function and other suggestions adam made.

[WebKit Review Bot]

Attachment 84163 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1

Tools/TestWebKitAPI/Tests/WebKit2/HitTestResultNodeHandle.cpp:47:  Missing space after ,  [whitespace/comma] [3]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alice Liu]

Created attachment 84164 [details]
same as above, but fixed whitespace style nit

[Gavin Barraclough]

Comment on attachment 84164 [details]
same as above, but fixed whitespace style nit

Looks great.

[Alice Liu]

committed http://trac.webkit.org/changeset/79966