Bug 139906 – [Win] Periodic failure in DumpRenderTree related to WebActionPropertyBag::Read

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Brent Fulgham]

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

FOLLOWUP_IP: 
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)

FAULT_INSTR_CODE:  c8a1e4b9

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

FAULTING_SOURCE_LINE_NUMBER:  115

FAULTING_SOURCE_CODE:  
   111:         return E_POINTER;
   112: 
   113:     VariantClear(pVar);
   114: 
>  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.

[Radar WebKit Bug Importer]

<rdar://problem/19337296>

[Brent Fulgham]

The stack trace for these errors typically looks like the following:

STACK_TEXT:  
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

[Brent Fulgham]

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.

[Brent Fulgham]

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.

[Brent Fulgham]

Created attachment 244912 [details]
Patch

[WebKit Commit Bot]

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.

[Anders Carlsson]

Comment on attachment 244912 [details]
Patch

Could we use COMVariant here?

[Brent Fulgham]

(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).

[Brent Fulgham]

Committed r178669: <http://trac.webkit.org/changeset/178669>