Bug 181801 – [Win] Null pointer crash under WebCore::RenderStyle::colorIncludingFallback.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Per Arne Vollan]

CONTEXT:  (.ecxr)
.ecxr
eax=00000001 ebx=00000000 ecx=00407724 edx=59d76bb4 esi=00000000 edi=00000000
eip=592bfb2a esp=004076c4 ebp=004076f8 iopl=0         nv up ei pl zr na pe nc
cs=001b  ss=0023  ds=0023  es=0023  fs=003b  gs=0000             efl=00210246
WebKit!WebCore::RenderStyle::backgroundColor [inlined in WebKit!WebCore::RenderStyle::colorIncludingFallback+0x7a]:
592bfb2a 8b4608          mov     eax,dword ptr [esi+8] ds:0023:00000008=????????
.cxr
Resetting default scope

FAULTING_IP: 
WebKit!WebCore::RenderStyle::colorIncludingFallback+7a
592bfb2a 8b4608          mov     eax,dword ptr [esi+8]

EXCEPTION_RECORD:  (.exr -1)
.exr -1
ExceptionAddress: 592bfb2a (WebKit!WebCore::RenderStyle::backgroundColor)
   ExceptionCode: c0000005 (Access violation)
  ExceptionFlags: 00000000
NumberParameters: 2
   Parameter[0]: 00000000
   Parameter[1]: 00000008
Attempt to read from address 00000008

DEFAULT_BUCKET_ID:  NULL_CLASS_PTR_READ

STACK_TEXT:  
004076f8 592bf420 00407724 0000002d 00000000 WebKit!WebCore::RenderStyle::colorIncludingFallback+0x7a
00407738 5939516f 00407764 0000002d 00000073 WebKit!WebCore::RenderStyle::visitedDependentColor+0x30
00407770 59394ab4 00000073 004077a0 0040779f WebKit!WebCore::RenderMenuList::getItemBackgroundColor+0x7f
004077ac 5954cb85 00407a40 00000073 0000000f WebKit!WebCore::RenderMenuList::itemStyle+0x84
00407b3c 5954e1e7 00407bb4 93011346 00000000 WebKit!WebCore::PopupMenuWin::paint+0x1e5
00407be8 5954e015 00070524 0000000f 00000000 WebKit!WebCore::PopupMenuWin::wndProc+0x197
00407c04 7639c4b7 00070524 0000000f 00000000 WebKit!WebCore::PopupMenuWin::PopupMenuWndProc+0x25
00407c30 76395f6f 5954dff0 00070524 0000000f user32!InternalCallWinProc+0x23
00407ca8 76394ede 00000000 5954dff0 00070524 user32!UserCallWinProcCheckWow+0xe0
00407d04 76394f4d 02f3bc08 0000000f 00000000 user32!DispatchClientMessage+0xcf
00407d2c 772d6bae 00407d44 00000018 0040eb34 user32!__fnDWORD+0x24
00407d58 76391bb4 7638ff95 00070524 00000060 ntdll!KiUserCallbackDispatcher+0x2e
00407d5c 7638ff95 00070524 00000060 00407d8c user32!NtUserCallHwndLock+0xc
00407d6c 5954c5a5 00070524 1d368de0 59444c7b user32!UpdateWindow+0x32
00407d78 59444c7b 1e123f40 1d368de0 00407e2c WebKit!WebCore::PopupMenuWin::updateFromElement+0x35
00407d8c 59441925 00000073 00000000 1e123f40 WebKit!WebCore::HTMLSelectElement::selectOption+0x10b
00407da4 59165d0a 1d368de0 1e123f40 092dd488 WebKit!WebCore::HTMLOptionElement::insertedInto+0x65
00407de8 59124222 1d368de0 1e123f40 00407e2c WebKit!WebCore::notifyNodeInsertedIntoDocument+0x2a
00407e04 590eacc4 1d368de0 1e123f40 00407e2c WebKit!WebCore::notifyChildNodeInserted+0x82
00407e68 590eaeef 1e123f40 00407e7c 00407ec0 WebKit!WebCore::ContainerNode::notifyChildInserted+0x84
00407e8c 590e806f 1e123f40 00000000 1e42f0e0 WebKit!WebCore::ContainerNode::updateTreeAfterInsertion+0x9f
00407f18 590e3442 00407f4c 1e123f40 1e22e570 WebKit!WebCore::ContainerNode::insertBefore+0x2bf
00407f2c 59a76a27 00407f4c 1e123f40 1e22e570 WebKit!WebCore::Node::insertBefore+0x32
00407f7c 59a74fbb 00407fa8 15a65b00 00407f90 WebKit!WebCore::JSDOMConstructorNotConstructable<WebCore::JSNode>::prototypeForStructure+0x12c7
00407f98 0c463edd 00407fa8 fffffffb 00408088 WebKit!WebCore::jsNodePrototypeFunctionInsertBefore+0x3b

[Per Arne Vollan]

<rdar://problem/35614900>

[Per Arne Vollan]

Created attachment 331638 [details]
Patch

[Per Arne Vollan]

Created attachment 331640 [details]
Patch

[zalan]

Comment on attachment 331640 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Check if html element in menu list is connected before getting its style.

In general, we should be able to access the style even when the element is detached. Looking at the stacktrace, the real issue here might be that WIN is too eager to paint a disconnected(?) popup (that should not happen for sure) and we end up accessing the computed style unexpectedly.

[Per Arne Vollan]

(In reply to zalan from comment #4)
> Comment on attachment 331640 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=331640&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        Check if html element in menu list is connected before getting its style.
> 
> In general, we should be able to access the style even when the element is
> detached. Looking at the stacktrace, the real issue here might be that WIN
> is too eager to paint a disconnected(?) popup (that should not happen for
> sure) and we end up accessing the computed style unexpectedly.

Thanks for reviewing! I'll look into fixing this on the Win side.

[Per Arne Vollan]

Created attachment 331819 [details]
Patch

[Per Arne Vollan]

Created attachment 331822 [details]
Patch

[Per Arne Vollan]

Created attachment 331823 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 331823 [details]
Patch

No layout test?

[Per Arne Vollan]

(In reply to Simon Fraser (smfr) from comment #9)
> Comment on attachment 331823 [details]
> Patch
> 
> No layout test?

I am actually close to having one, but I have not been able to open a popup with JavaScript yet which is required to reproduce this. I partially succeeded by opening the popup with a mouse down from window.eventsender, but the events are synchronous in the Windows event sender, which means the mouse down event will block further JS execution, since the mouse down will start the popup event loop, and will not return from there until the popup is closed. Perhaps there are other ways to open a popup with JavaScript?

[Per Arne Vollan]

Created attachment 331931 [details]
Patch

[Per Arne Vollan]

(In reply to Per Arne Vollan from comment #11)
> Created attachment 331931 [details]
> Patch

Added an explanation of why I have not added a new test.

[Brent Fulgham]

Comment on attachment 331931 [details]
Patch

r=me.

[Per Arne Vollan]

Comment on attachment 331931 [details]
Patch

Thanks for reviewing!

[WebKit Commit Bot]

Comment on attachment 331931 [details]
Patch

Clearing flags on attachment: 331931

Committed r227357: <https://trac.webkit.org/changeset/227357>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.