Bug 139906 - [Win] Periodic failure in DumpRenderTree related to WebActionPropertyBag::Read
Summary: [Win] Periodic failure in DumpRenderTree related to WebActionPropertyBag::Read
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brent Fulgham
Keywords: InRadar
Depends on:
Reported: 2014-12-23 11:08 PST by Brent Fulgham
Modified: 2015-01-19 12:58 PST (History)
3 users (show)

See Also:

Crash log for 'dom/html/level2/html/HTMLIFrameElement03.html' (28.92 KB, text/plain)
2014-12-23 11:08 PST, Brent Fulgham
no flags Details
Patch (22.86 KB, patch)
2015-01-19 11:33 PST, Brent Fulgham
dino: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2014-12-23 11:08:19 PST
Created attachment 243680 [details]
Crash log for 'dom/html/level2/html/HTMLIFrameElement03.html'

A number of tests fail (unreliably) due to some kind of heap corruption that manifests as a crash in WebActionPropertyBag:

STACK_COMMAND:  .ecxr ; kb ; dps 774842a8 ; kb

WebKit!WebActionPropertyBag::Read+28 [c:\cygwin\home\buildbot\slave\win-release\build\source\webkit\win\webactionpropertybag.cpp @ 115]
021da358 b9e4a1c802      mov     ecx,offset WebKit!piOverFourDouble+0xd4 (02c8a1e4)


FAULTING_SOURCE_LINE:  c:\cygwin\home\buildbot\slave\win-release\build\source\webkit\win\webactionpropertybag.cpp

FAULTING_SOURCE_FILE:  c:\cygwin\home\buildbot\slave\win-release\build\source\webkit\win\webactionpropertybag.cpp


   111:         return E_POINTER;
   113:     VariantClear(pVar);
>  115:     if (isEqual(pszPropName, WebActionNavigationTypeKey)) {
   116:         V_VT(pVar) = VT_I4;
   117:         V_I4(pVar) = m_action.type();
   118:         return S_OK;
   119:     }
   120:     if (isEqual(pszPropName, WebActionElementKey)) {

See the attached debug dump.
Comment 1 Radar WebKit Bug Importer 2014-12-23 11:08:49 PST
Comment 2 Brent Fulgham 2015-01-19 10:44:35 PST
The stack trace for these errors typically looks like the following:

77c742a8 77bf9cb1 ntdll!RtlSizeHeap+0x69
77c742ac 75c6625c ole32!CRetailMalloc_GetSize+0x21
77c742b0 773b443a oleaut32!APP_DATA::FreeCachedMem+0x30
77c742b4 773b3ea3 oleaut32!SysFreeString+0x6b
77c742b8 773b4870 oleaut32!VariantClear+0xc3
77c742bc 022ef828 webkit!WebActionPropertyBag::Read+0x28
77c742c0 022e4642 webkit!DefaultPolicyDelegate::decidePolicyForNavigationAction+0x22
77c742c4 022fa206 webkit!WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction+0xf6
77c742c8 029ef146 webkit!WebCore::PolicyChecker::checkNavigationPolicy+0x226
77c742cc 023ad33b webkit!WebCore::FrameLoader::loadWithDocumentLoader+0x30b
77c742d0 023ad483 webkit!WebCore::FrameLoader::load+0xd3
77c742d4 023a6c2c webkit!WebCore::FrameLoader::load+0x19c
77c742d8 02306805 webkit!WebFrame::loadRequest+0x95
77c742dc 10015c12 dumprendertree!runTest+0x672
77c742e0 100163e0 dumprendertree!main+0x2d0
77c742e4 1001652e dumprendertree!dllLauncherEntryPoint+0xe
77c742e8 00401c35 dumprendertree!main+0x4b5
77c742ec 004042f9 dumprendertree!__tmainCRTStartup+0xfe
77c742f0 76fe338a kernel32!BaseThreadInitThunk+0xe
77c742f4 77ba9f72 ntdll!__RtlUserThreadStart+0x70
77c742f8 77ba9f45 ntdll!_RtlUserThreadStart+0x1b
Comment 3 Brent Fulgham 2015-01-19 10:45:56 PST
MSDN clearly states: "Do not use VariantClear on unitialized variants; use VariantInit to initialize a new VARIANTARG or VARIANT"

In the call stack for these crashes, I typically see an uninitialized variant value.

We should either (a) switch from raw VARIANT values to _variant_t classes (much like _bstr_t), or (b) ensure that ::VariantInit is called each time we create a new VARIANT variable.
Comment 4 Brent Fulgham 2015-01-19 11:14:50 PST
Calling ::VariantClear on uninitialized memory is bad, because it attempts to interpret the meaning of the current contents of the VARIANT passed to it. If this is uninitialized memory, it can make bad assumptions about the content of the VARIANT.
Comment 5 Brent Fulgham 2015-01-19 11:33:28 PST
Created attachment 244912 [details]
Comment 6 WebKit Commit Bot 2015-01-19 11:34:58 PST
Attachment 244912 [details] did not pass style-queue:

ERROR: Source/WebKit/win/DefaultPolicyDelegate.cpp:32:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 16 files

If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Anders Carlsson 2015-01-19 11:37:42 PST
Comment on attachment 244912 [details]

Could we use COMVariant here?
Comment 8 Brent Fulgham 2015-01-19 11:51:10 PST
(In reply to comment #7)
> Comment on attachment 244912 [details]
> Patch
> Could we use COMVariant here?

Yes, except the changes I need to make are mostly in DumpRenderTree. COMVariant is internal to WebKit, and is not available to DRT (unless we change the set of files we ship with WebKit).
Comment 9 Brent Fulgham 2015-01-19 12:58:03 PST
Committed r178669: <http://trac.webkit.org/changeset/178669>