Bug 91116 – WebContext::getWebCoreStatistics() causes crash if no m_process

• Component: WebKit2
• Status: RESOLVED FIXED

[Josh Hawn]

1 com.apple.WebKit2              0x1090cc9e7 WebKit::WebProcessProxy::sendMessage(CoreIPC::MessageID, WTF::PassOwnPtr<CoreIPC::ArgumentEncoder>, unsigned int) + 0xb
>  2 com.apple.WebKit2              0x10916dc5e bool WebKit::WebProcessProxy::send<Messages::WebProcess::GetWebCoreStatistics>(Messages::WebProcess::GetWebCoreStatistics const&, unsigned long long, unsigned int) + 0x6a
    3 com.apple.WebKit2              0x10916d0b1 WebKit::WebContext::getWebCoreStatistics(WTF::PassRefPtr<WebKit::GenericCallback<OpaqueWKDictionary const*, WebKit::ImmutableDictionary*> >) + 0x6f
    4 com.apple.WebKit2              0x1091cf413 WKContextGetStatistics + 0x66

When trying to send this message, this function does not check or ensure that there is a valid m_process pointer.

<rdar://problem/11834373>

[Josh Hawn]

Created attachment 151999 [details]
Proposed patch which checks for m_process before sending message.

Return value has also been changed from void to bool. Because send() returns a bool, it is probably a good idea to return this result up to the user of the function. This way the user will know if the action succeeded or not.

[Josh Hawn]

Created attachment 152013 [details]
Proposed patch which checks for m_process before sending message.

[Jessie Berlin]

Comment on attachment 152013 [details]
Proposed patch which checks for m_process before sending message.

You are missing a ChangeLog. I think you can also write an API test for this.

[Josh Hawn]

Created attachment 152063 [details]
Proposed patch with test case.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebKit2/UIProcess/Web..." exit_code: 1
Tools/TestWebKitAPI/Tests/WebKit2/WebCoreStatisticsWithNoWebProcess.cpp:32:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 5 files


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

[Josh Hawn]

Created attachment 152070 [details]
Proposed patch with test case.

[Simon Fraser (smfr)]

Comment on attachment 152070 [details]
Proposed patch with test case.

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

> Source/WebKit2/UIProcess/WebContext.cpp:-923
>  void WebContext::getWebCoreStatistics(PassRefPtr<DictionaryCallback> prpCallback)
>  {
> -    RefPtr<DictionaryCallback> callback = prpCallback;

Ugh, we don't do hungarian notation.

This method is really confusing. It's called getWebCoreStatistics() but doesn't return anything.

> Source/WebKit2/UIProcess/WebContext.cpp:930
> +    RefPtr<DictionaryCallback> callback = prpCallback;
>      uint64_t callbackID = callback->callbackID();
>      m_dictionaryCallbacks.set(callbackID, callback.get());

I think this RefPtr is doing an extra retain. Won't m_dictionaryCallbacks have the owning reference?

[Simon Fraser (smfr)]

Comment on attachment 152070 [details]
Proposed patch with test case.

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

>> Source/WebKit2/UIProcess/WebContext.cpp:930
>>      m_dictionaryCallbacks.set(callbackID, callback.get());
> 
> I think this RefPtr is doing an extra retain. Won't m_dictionaryCallbacks have the owning reference?

I would guess you want:

m_dictionaryCallbacks.set(callbackID, prpCallback.release());

but beware that prpCallback will be nulled out after this.

[Josh Hawn]

(In reply to comment #8)
>> I think this RefPtr is doing an extra retain. Won't m_dictionaryCallbacks have the owning reference?
>
>I would guess you want:
>
>m_dictionaryCallbacks.set(callbackID, prpCallback.release());
>
>but beware that prpCallback will be nulled out after this.
>

The RefPtr assignment doesn't do an extra retain when it's assigned from a PassRefPtr.
The release() method is only defined on RefPtr, not PassRefPtr, and returns a PassRefPtr (after nulling its internal pointer without changing the reference count), not the raw pointer that HashMap.set(key, value) requires.

how about this instead:

void WebContext::getWebCoreStatistics(PassRefPtr<DictionaryCallback> callback)
{
    if (!m_process) {
        callback->invalidate();
        return;
    }
    
    uint64_t callbackID = callback->callbackID();
    m_dictionaryCallbacks.set(callbackID, callback.get());
    process()->send(Messages::WebProcess::GetWebCoreStatistics(callbackID), 0);
}


I've used this guideline for using PassRefPtr is function arguments:
    "If a function does take ownership of an object, the argument should be a PassRefPtr. This includes most setter functions. Unless the use of the argument is very simple, the argument should be transferred to a RefPtr at the start of the function; the argument can be named with a “prp” prefix in such cases."

Since the use of this argument is fairly simple, we can do without transferring it to a RefPtr, and no longer need the 'prp' prefix. I believe this is still using it correctly since callback isn't being assigned to any other local variable or being passed as an argument to any other function. And, as you mentioned earlier, m_dictionaryCallbacks will have the owning reference. Let me know if I'm still understanding this wrong.

[Josh Hawn]

Created attachment 152275 [details]
Proposed patch with test case. (Minor change after review)

The above comment explains the change from the last patch.

[WebKit Review Bot]

Comment on attachment 152275 [details]
Proposed patch with test case. (Minor change after review)

Clearing flags on attachment: 152275

Committed r122648: <http://trac.webkit.org/changeset/122648>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.