Bug 93128 – [WIN] Use BString in favour of BSTR to improve memory management

• Component: WebKit API
• Status: RESOLVED FIXED

[Patrick R. Gansterer]

[WIN] Use BString in favour of BSTR to improve memory management

[Patrick R. Gansterer]

Created attachment 156394 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 156394 [details]
Patch

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

> Source/WebCore/platform/win/BString.cpp:130
> -    if (m_bstr)
> -        SysFreeString(m_bstr);
> +    clear();

Is SysFreeString null-safe?  The previous code didn't seem to think so.

[Patrick R. Gansterer]

Comment on attachment 156394 [details]
Patch

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

>> Source/WebCore/platform/win/BString.cpp:130
>> +    clear();
> 
> Is SysFreeString null-safe?  The previous code didn't seem to think so.

Yes it is. The previous code did only on a few places.

[WebKit Review Bot]

Comment on attachment 156394 [details]
Patch

Clearing flags on attachment: 156394

Committed r128809: <http://trac.webkit.org/changeset/128809>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by 96958

[Patrick R. Gansterer]

Created attachment 164621 [details]
Patch

[WebKit Review Bot]

Comment on attachment 164621 [details]
Patch

Rejecting attachment 164621 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/13898489

[Patrick R. Gansterer]

Created attachment 164707 [details]
Patch

[WebKit Review Bot]

Comment on attachment 164707 [details]
Patch

Clearing flags on attachment: 164707

Committed r128988: <http://trac.webkit.org/changeset/128988>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Roger Fong]

Hello, this patch is causing an assertion failure on Windows. All I'm doing is launching WinLauncher.

I get an assertion failure ASSERTION FAILED !m_bstr.

Here's the stack trace.


 	WebKit.dll!WebCore::BString::operator&()  Line 61 + 0x3a bytes	C++
>	WebKit.dll!WebView::notifyPreferencesChanged(IWebNotification * notification=0x005fefb8)  Line 4591 + 0x16 bytes	C++
 	WebKit.dll!WebView::onNotify(IWebNotification * notification=0x005fefb8)  Line 4549 + 0xf bytes	C++
 	WebKit.dll!WebNotificationCenter::postNotificationInternal(IWebNotification * notification=0x005fefb8, wchar_t * notificationName=0x004fb9bc, IUnknown * anObject=0x005be918)  Line 131 + 0x14 bytes	C++
 	WebKit.dll!WebNotificationCenter::postNotificationName(wchar_t * notificationName=0x004fb9bc, IUnknown * anObject=0x005be918, IPropertyBag * userInfo=0x00000000)  Line 188	C++
 	WebKit.dll!WebPreferences::postPreferencesChangesNotification()  Line 144 + 0x1c bytes	C++
 	WebKit.dll!WebView::initWithFrame(tagRECT frame={...}, wchar_t * frameName=0x00000000, wchar_t * groupName=0x00000000)  Line 2726	C++
 	WinLauncher.dll!dllLauncherEntryPoint(HINSTANCE__ * __formal=0x00120000, HINSTANCE__ * __formal=0x00120000, HINSTANCE__ * __formal=0x00120000, int nCmdShow=1)  Line 368 + 0x35 bytes	C++
 	WinLauncher.exe!wWinMain(HINSTANCE__ * hInstance=0x00120000, HINSTANCE__ * hPrevInstance=0x00000000, wchar_t * lpstrCmdLine=0x004a4afe, int nCmdShow=1)  Line 208 + 0x18 bytes	C++
 	WinLauncher.exe!__tmainCRTStartup()  Line 589 + 0x1c bytes	C
 	kernel32.dll!@BaseThreadInitThunk@12()  + 0x12 bytes	
 	ntdll.dll!___RtlUserThreadStart@8()  + 0x27 bytes	
 	ntdll.dll!__RtlUserThreadStart@8()  + 0x1b bytes	

Do you mind taking a look, it's preventing my from debugging things...
Thanks!

[Roger Fong]

(In reply to comment #12)
> Hello, this patch is causing an assertion failure on Windows. All I'm doing is launching WinLauncher.
> 
> I get an assertion failure ASSERTION FAILED !m_bstr.
> 
> Here's the stack trace.
> 
> 
>      WebKit.dll!WebCore::BString::operator&()  Line 61 + 0x3a bytes    C++
> >	WebKit.dll!WebView::notifyPreferencesChanged(IWebNotification * notification=0x005fefb8)  Line 4591 + 0x16 bytes	C++
>      WebKit.dll!WebView::onNotify(IWebNotification * notification=0x005fefb8)  Line 4549 + 0xf bytes    C++
>      WebKit.dll!WebNotificationCenter::postNotificationInternal(IWebNotification * notification=0x005fefb8, wchar_t * notificationName=0x004fb9bc, IUnknown * anObject=0x005be918)  Line 131 + 0x14 bytes    C++
>      WebKit.dll!WebNotificationCenter::postNotificationName(wchar_t * notificationName=0x004fb9bc, IUnknown * anObject=0x005be918, IPropertyBag * userInfo=0x00000000)  Line 188    C++
>      WebKit.dll!WebPreferences::postPreferencesChangesNotification()  Line 144 + 0x1c bytes    C++
>      WebKit.dll!WebView::initWithFrame(tagRECT frame={...}, wchar_t * frameName=0x00000000, wchar_t * groupName=0x00000000)  Line 2726    C++
>      WinLauncher.dll!dllLauncherEntryPoint(HINSTANCE__ * __formal=0x00120000, HINSTANCE__ * __formal=0x00120000, HINSTANCE__ * __formal=0x00120000, int nCmdShow=1)  Line 368 + 0x35 bytes    C++
>      WinLauncher.exe!wWinMain(HINSTANCE__ * hInstance=0x00120000, HINSTANCE__ * hPrevInstance=0x00000000, wchar_t * lpstrCmdLine=0x004a4afe, int nCmdShow=1)  Line 208 + 0x18 bytes    C++
>      WinLauncher.exe!__tmainCRTStartup()  Line 589 + 0x1c bytes    C
>      kernel32.dll!@BaseThreadInitThunk@12()  + 0x12 bytes    
>      ntdll.dll!___RtlUserThreadStart@8()  + 0x27 bytes    
>      ntdll.dll!__RtlUserThreadStart@8()  + 0x1b bytes    
> 
> Do you mind taking a look, it's preventing my from debugging things...
> Thanks!

In notifyPreferencesChanged, it looks like the bstr gets set on     
settings->setCursiveFontFamily(toAtomicString(str));.

Later a reference to said bstr gets passed into     
hr = preferences->defaultTextEncodingName(&str);

For some reason the &operator for bstrings is overriden to assert(!b_str). I'm not sure why though...

[Patrick R. Gansterer]

Committed r129598: <http://trac.webkit.org/changeset/129598>