Bug 187167 – [WinCairo] WebKit MiniBrowser crashes when attempting to navigate to certain URLs

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Christopher Reid]

It is crashing due to mishandling of buffers when doing string conversion.

[Christopher Reid]

Created attachment 343887 [details]
Patch

[Fujii Hironori]

Comment on attachment 343887 [details]
Patch

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

I'm not a reviewer. This is an informal review.

> Tools/MiniBrowser/win/WebKitBrowserWindow.cpp:48
> +std::vector<char> toUtf8(const wchar_t* src, size_t srcLength)

Why do you want to use std::vector<char> for utf-8 strings?

> Tools/MiniBrowser/win/WebKitBrowserWindow.cpp:233
> +    SetWindowText(thisWindow.m_urlBarWnd, urlString.data());

Why did you replace c_str with data? Windows API expects a null-terminated string.

[Christopher Reid]

(In reply to Fujii Hironori from comment #2)
> Comment on attachment 343887 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=343887&action=review
> 
> I'm not a reviewer. This is an informal review.
> 
> > Tools/MiniBrowser/win/WebKitBrowserWindow.cpp:48
> > +std::vector<char> toUtf8(const wchar_t* src, size_t srcLength)
> 
> Why do you want to use std::vector<char> for utf-8 strings?

This function was copying the buffer to a std::string just for callers to call c_str() on the returned string.
I wanted to use std::vector here as that copy didn't seem needed.

> > Tools/MiniBrowser/win/WebKitBrowserWindow.cpp:233
> > +    SetWindowText(thisWindow.m_urlBarWnd, urlString.data());
> 
> Why did you replace c_str with data? Windows API expects a null-terminated
> string.

Oops right, those ones shouldn't have been changed.

[Christopher Reid]

Created attachment 343896 [details]
Patch

[Fujii Hironori]

Super. LGTM.

[Anders Carlsson]

Comment on attachment 343896 [details]
Patch

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

> Tools/MiniBrowser/win/WebKitBrowserWindow.cpp:62
> -    return adoptWK(WKStringCreateWithUTF8CString(utf8.c_str()));
> +    return adoptWK(WKStringCreateWithUTF8CString(utf8.data()));

Pretty sure WKStringCreateWithUTF8CString expects a null terminated string.

> Tools/MiniBrowser/win/WebKitBrowserWindow.cpp:69
> -    return adoptWK(WKURLCreateWithUTF8CString(utf8.c_str()));
> +    return adoptWK(WKURLCreateWithUTF8CString(utf8.data()));

Same here.

[Michael Catanzaro]

(In reply to Fujii Hironori from comment #2)
> > Tools/MiniBrowser/win/WebKitBrowserWindow.cpp:233
> > +    SetWindowText(thisWindow.m_urlBarWnd, urlString.data());
> 
> Why did you replace c_str with data? Windows API expects a null-terminated
> string.

Note that for std::string, c_str() and data() are equivalent since C++ 11 (i.e. data() is now guaranteed to be null-terminated).

[Christopher Reid]

(In reply to Anders Carlsson from comment #6)
> Comment on attachment 343896 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=343896&action=review
> 
> > Tools/MiniBrowser/win/WebKitBrowserWindow.cpp:62
> > -    return adoptWK(WKStringCreateWithUTF8CString(utf8.c_str()));
> > +    return adoptWK(WKStringCreateWithUTF8CString(utf8.data()));
> 
> Pretty sure WKStringCreateWithUTF8CString expects a null terminated string.

I think that's okay, toUtf8() is null terminating the buffer.

[Alex Christensen]

Comment on attachment 343896 [details]
Patch

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

> Tools/MiniBrowser/win/WebKitBrowserWindow.cpp:48
> +std::vector<char> toUtf8(const wchar_t* src, size_t srcLength)

toNullTerminatedUTF8?

[Christopher Reid]

Created attachment 344449 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 344449 [details]
Patch for landing

Rejecting attachment 344449 [details] from commit-queue.

New failing tests:
media/media-fullscreen-return-to-inline.html
Full output: https://webkit-queues.webkit.org/results/8459655

[WebKit Commit Bot]

Created attachment 344455 [details]
Archive of layout-test-results from webkit-cq-03 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: webkit-cq-03  Port: mac-sierra  Platform: Mac OS X 10.12.6

[WebKit Commit Bot]

Comment on attachment 344449 [details]
Patch for landing

Clearing flags on attachment: 344449

Committed r233610: <https://trac.webkit.org/changeset/233610>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/41918558>