Bug 76977 – Check if inner node is null in order to avoid crash.

• Component: UI Events
• Status: NEW

[Gyuyoung Kim]

It is missing to check if inner node is null in touch event handler of event handler. It may cause crash when page is loading. Especially, inner node can be null on slow wireless network. In addition, we should check if inner node is null by default when we get an inner node from HitTestResult.

[Gyuyoung Kim]

Created attachment 123877 [details]
Patch

[Andreas Kling]

Comment on attachment 123877 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        It is missing to check if inner node is null in touch event handler of event handler. It may cause
> +        crash when page is loading. Especially, inner node can be null on slow wireless network.

Why can the inner node be null on slow wireless networks?

> Source/WebCore/ChangeLog:12
> +        No new tests. Speculative fix for crash.

We're gonna need a test, or an explanation of why a test is not possible.

[Gyuyoung Kim]

Created attachment 124038 [details]
Patch

[Gyuyoung Kim]

(In reply to comment #2)
> (From update of attachment 123877 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=123877&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        It is missing to check if inner node is null in touch event handler of event handler. It may cause
> > +        crash when page is loading. Especially, inner node can be null on slow wireless network.
> 
> Why can the inner node be null on slow wireless networks?

I thought that wireless network is much longer page loading time than normal network. So, likely to crash. But, it looks it is unneeded comment. I remove it.
 
> > Source/WebCore/ChangeLog:12
> > +        No new tests. Speculative fix for crash.
> 
> We're gonna need a test, or an explanation of why a test is not possible.

I have tried to implement test case since last weekend. But, it was very difficult to reproduce inner node is null. Do you know how to reproduce it ? IMHO, it is clear to check inner node is null. When I make proper test case for this patch, I will submit it to Bugzilla again.

[Gyuyoung Kim]

CC'ing kling.

[Antonio Gomes]

Comment on attachment 124038 [details]
Patch

we have this patch in the blackberry port as well, and iirc it was me who added. I was able to come up a layout test to this.

[Antonio Gomes]

(In reply to comment #6)
> (From update of attachment 124038 [details])
> we have this patch in the blackberry port as well, and iirc it was me who added. I was able to come up a layout test to this.

so patch itself it valid.

[Antonio Gomes]

Created attachment 124257 [details]
layout test used to trigger the crash

[Antonio Gomes]

Created attachment 124258 [details]
iframe (withing resources) used in the previous html

[Antonio Gomes]

Created attachment 124259 [details]
xml used

[Antonio Gomes]

Created attachment 124260 [details]
xsl used

[Antonio Gomes]

this the attachments I managed to reproduce the crash. It was not usually due to slow network, but instead slow parsing on embedded device hardware.

[Andreas Kling]

(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 124038 [details] [details])
> > we have this patch in the blackberry port as well, and iirc it was me who added. I was able to come up a layout test to this.
> 
> so patch itself it valid.

Right, great that you have a test!

The issue I have with this change is that we don't seem to know why we need the null check. Do we know what's causing the inner node to be null here? Do we have a backtrace for the crash?

[Antonio Gomes]

(In reply to comment #13)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > (From update of attachment 124038 [details] [details] [details])
> > > we have this patch in the blackberry port as well, and iirc it was me who added. I was able to come up a layout test to this.
> > 
> > so patch itself it valid.
> 
> Right, great that you have a test!
> 
> The issue I have with this change is that we don't seem to know why we need the null check. Do we know what's causing the inner node to be null here? Do we have a backtrace for the crash?

in our case we were hit test a to-be-constructed LayerTree. let me try to get the details...

[Antonio Gomes]

bt in our case:

Program received signal SIGSEGV, Segmentation fault.
[Switching to pid 5050394 tid 13 name "webkit_main"]
WebCore::EventHandler::handleTouchEvent (this=0x7cb7e750, event=...) at
/home/cswei/project/playbook/webkit/Source/WebCore/page/EventHandler.cpp:3225
3225            if (node->isTextNode())
(gdb) print nde
No symbol "nde" in current context.
(gdb) print node
$1 = (WebCore::Node *) 0x0
(gdb) bt

#0  WebCore::EventHandler::handleTouchEvent (this=0x7cb7e750, event=...) at
/home/cswei/project/playbook/webkit/Source/WebCore/page/EventHandler.cpp:3225
#1  0x7e9aa128 in BlackBerry::WebKit::WebPage::touchEvent (this=0x7aef6728,
event=...)
    at
/home/cswei/project/playbook/webkit/Source/WebKit/blackberry/Api/WebPage.cpp:3796
#2  0x7cdb1f60 in TouchHandlerWebKitThread::onTouchEvent (this=0x7cb73e18,
event=..., permissions=<optimized out>)
    at /home/cswei/project/playbook/libwebview/TouchHandlerWebKitThread.cpp:113
#3  0x7cdbffec in BlackBerry::Platform::MethodDelegate2<void
(TouchHandlerWebKitThread::*)(BlackBerry::Platform::TouchEvent const&,
TouchPermissions const&)
(...)


the ContentLayer we were doing the hit test has was very empty still, since it is still building the document (iirc, still parsing the long xml/xsl):

layer 848a3c at (0,0) size 0x0
  RenderView 848960 at (0,0) size 0x0

[Gyuyoung Kim]

Great !!

Antonio, will you submit new patch with test cases ?

[Antonio Gomes]

I am sure Qtwebkit1 could be used to test this, but I failed to actually build it with xsl support ON, so the long parse-able xml+xsl would actually take long to parse. I can give another try now that I saw recent improvements on this area.

Does webkit/efl render complex xml webpages well?

kling, would you be happy with such a brutal force layout test? :-)

[Gyuyoung Kim]

(In reply to comment #17)

> Does webkit/efl render complex xml webpages well?

Though I don't test complex xml webpage on EFL port yet, AFAIK, WebKit EFL can render complex xml webpage as well. But, I'm not sure if EFL DRT can support this test case. Because there are still missing implementations in EFL DRT.