| Summary: | [WIN] Use BString in favour of BSTR to improve memory management | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Patrick R. Gansterer <paroga> | ||||||||
| Component: | WebKit API | Assignee: | Patrick R. Gansterer <paroga> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | andersca, aroben, bfulgham, hausmann, rniwa, roger_fong, webkit.review.bot | ||||||||
| Priority: | P2 | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | PC | ||||||||||
| OS: | Windows XP | ||||||||||
| Bug Depends on: | 96958 | ||||||||||
| Bug Blocks: | |||||||||||
| Attachments: |
|
||||||||||
|
Description
Patrick R. Gansterer
2012-08-03 08:56:39 PDT
Created attachment 156394 [details]
Patch
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. 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. Comment on attachment 156394 [details] Patch Clearing flags on attachment: 156394 Committed r128809: <http://trac.webkit.org/changeset/128809> All reviewed patches have been landed. Closing bug. Re-opened since this is blocked by 96958 Created attachment 164621 [details]
Patch
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 Created attachment 164707 [details]
Patch
Comment on attachment 164707 [details] Patch Clearing flags on attachment: 164707 Committed r128988: <http://trac.webkit.org/changeset/128988> All reviewed patches have been landed. Closing bug. 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 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... Committed r129598: <http://trac.webkit.org/changeset/129598> |