Bug 56393

Created attachment 85929 [details] Check the existence of the renderer of element, before trying to access the layer. This patch provides a check for existence of the renderer of element, before trying to access the enclosing layer.

Comment on attachment 85929 [details] Check the existence of the renderer of element, before trying to access the layer. How do we test this?

Comment on attachment 85929 [details] Check the existence of the renderer of element, before trying to access the layer. View in context: https://bugs.webkit.org/attachment.cgi?id=85929&action=review > Source/WebCore/ChangeLog:6 > + Without checking whether renderer of the element is existing, it is trying to find the layer. s/is existing/exists/

Suchi sent me this in email: I am wondering if you could help me out about the testcase for bug #56393, you had denied to review the patch as I did not provide the testcase. As this is just a NULL check, I am not able to create layout testcase for this yet. If renderer is null on a generic case, the control does not even come to this function and will crash much beyond. I see the existence of same NULL check in function RenderView::paintBoxDecorations() in file RenderView.cpp. If you could let me know any existense test case for the same NULL check would be great. My response: So I'm confused why we're adding this null check if this is never null? What case is it ever null that we need this null check for? Do you have an example page which crashes?

Hi Eric, Browser crashed on loading www.crave.cnet.com. During debug we have found out that without checking whether renderer of the element existed, it tried to find the layer and as a result the crash happened. With this NULL check we were able to prevent the crash.

Hi Eric, Browser crashed on loading www.crave.cnet.com. During debug we have found out that without checking whether renderer of the element existed, it tried to find the layer and as a result the crash happened. With this NULL check we were able to prevent the crash.

Created attachment 89185 [details] Modified changelog As this Null check does not change the functionality, no new layout test was created for this change.

Comment on attachment 89185 [details] Modified changelog You seem very confused about this change. First, understand what is going on, should the layer be null here, if yes in which cases. Then make a layout test accessing this path. If that crashes on some websites, it is likely possible to make a test for it.

(In reply to comment #6) > Hi Eric, Browser crashed on loading www.crave.cnet.com. During debug we have found out that without checking whether renderer of the element existed, it tried to find the layer and as a result the crash happened. With this NULL check we were able to prevent the crash. Did you try to create reduction? Some simple html that crashes webkit. If you get reduction it would be very easy to add layout test. There is a lot of tests that pass if webkit doesn’t crash. Does it crash with frame flattening switched on?

Blocking 2.1.x as requested/discussed via e-mail. Any chance of adding the test still this week? I would like to avoid adding yet another unreviewed patch to 2.1.x.

Patch added to 2.1 as c56e63a70e7186cd056b192f8a0056f6ed3bc84d, pending trunk inclusion and blocking 2.2. P1 because it's a crash.

This is not resetting on Linux because it allows calling a nonvirtual function with a null this pointer. If I add this assert statement it will reset on Linux too. Source/WebCore/page/FrameView.cpp ASSERT(elt->renderer()); layer = elt->renderer()->enclosingLayer(); The Sample html files to reproduce the crash in https://bugs.webkit.org/show_bug.cgi?id=59684 show a small test case. We should be able add a layout test now.

Created attachment 94067 [details] Example Layout test for this problem I am not only submitting this as a an example for a test. I think it has too many problems listed below for it to be a real test. Main problem is that I could not get it to not reset on Linux QtWebkit with gtk/flash plugin interaction. Here is a small test case that can show the null renderer reset. It contains an iframe with style="display:none" of an object element of a .swf file. There are a number of things I don't like about this test, but could not figure out a better way to do. First, I had to add an ASSERT((elt->renderer()) to FrameView::windowClipRect() to get it to reset in Linux as it does on Symbian. Second, I had to break the test into 3 files. I could not reproduce the problem by trying to embed content with the data: scheme. Third, since I have to wait for the iframe file and .swf file to load, I used a window.setTimeout( "bodyLoaded();", 300) to wait for the failure. Fourth, I had to go through all sorts of investigation to get the plugin code in be loaded under run-webkit-tests to reproduce this error. Eventually, I just copied the Linux plugins to the run-webkit-tests plugin area with $ cp /usr/lib/mozilla/plugins/*.so /home/jwild/dev/webkit/WebKit.2011.03.21/WebKitBuild/Release/lib/plugins/ Fifth, this test was continuing to assert/reset on Linux QtWebkit because of problem with the Flash Plugin/Gtk that fails for other .swf tests. (process:27607): Gtk-CRITICAL **: IA__gtk_clipboard_get_for_display: assertion `display != NULL' failed Adobe Flash Player: gtk_clipboard_get(GDK_SELECTION_PRIMARY); failed. Trying to call gtk_init(0,0); (<unknown>:27607): Gdk-CRITICAL **: IA__gdk_window_get_origin: assertion `GDK_IS_WINDOW (window)' failed (<unknown>:27607): Gdk-WARNING **: /build/buildd/gtk+2.0-2.22.0/gdk/x11/gdkdrawable-x11.c:952 drawable is not a pixmap or window Source:

Chris is Mr-Layer. He might have a sense as to if this is "expected" or not. I think Suchi just needs to spend a bit of time and come up with a reduction from the original site. Code changes are basically useless w/o testing, as WebKit changes soo fast that we commonly regress untested fixes (hence why we require tests when possible).

(In reply to comment #14) > Chris is Mr-Layer. He might have a sense as to if this is "expected" or not. I think Suchi just needs to spend a bit of time and come up with a reduction from the original site. Code changes are basically useless w/o testing, as WebKit changes soo fast that we commonly regress untested fixes (hence why we require tests when possible). Does the test submitted in Comment #13 seem adequate? I know it has a lot of limitations as I listed, but I'm not sure how to make it better or smaller since it relies on the plugin environment. If you think it is adequate, we can submit a patch with both Suchi's fix and the test.

Created attachment 96287 [details] FrameView null renderer fix for WebKit 2.2 Here is Suchi's original patch applied to WebKit 2.2. It is not submitted for review. It is only here for a convenience if it is needed to be applied to 2.2.

(In reply to comment #16) > Created an attachment (id=96287) [details] > FrameView null renderer fix for WebKit 2.2 > > Here is Suchi's original patch applied to WebKit 2.2. It is not submitted for review. > It is only here for a convenience if it is needed to be applied to 2.2. So we're giving up by now? :-(

I am able to get this to fail more cleanly with the WebKit Gtk build. I will submit a new patch with the patched code and test case for review. Just running tests now.

Created attachment 96602 [details] Check for null renderer to avoid reset. I was able to get the layout test to run more cleanly under Gtk WebKit than Qt WebKit because Gtk seems to have a full plugin code for DumpRenderTree. Included is that code fix and layout test.

Comment on attachment 96602 [details] Check for null renderer to avoid reset. View in context: https://bugs.webkit.org/attachment.cgi?id=96602&action=review > Source/WebCore/rendering/RenderObject.cpp:528 > + ASSERT(this); I don't think this is the right fix. How come the "this" could be null here? In which scenario, the "this" could be null? I don't think you fix the root of the problem here. I don't understand why this change is needed.

(In reply to comment #20) The "this" pointer is null on all platforms (at least the ones I looked at). It only reset's on Symbian because of a difference in execution environment. The renderer is null causing a null "this". This is show in the small test case by an iframe with style=display:none to an html page containing a .swf plugin. I will look again in the debugger and see if I can exactly where and why the renderer is set to null.

(In reply to comment #21) > (In reply to comment #20) > > The "this" pointer is null on all platforms (at least the ones I looked at). > It only reset's on Symbian because of a difference in execution environment. > > The renderer is null causing a null "this". This is show in the small test case by an iframe with style=display:none to an html page containing a .swf > plugin. > > I will look again in the debugger and see if I can exactly where and why the > renderer is set to null. But the test should be done before the function call, i.e. before enclosingLayer() is called like the other part of your patch does. ASSERT is not run in release so what's the purpose of the ASSERT, to check you don't enter in enclosingLayer() with an invalid pointer. It's not the right way, you should ASSERT *before* the call.

(In reply to comment #22) Now I understand your question. I added this ASSERT so that this situation would fail on all platforms so I could write the test. If we had Symbian layout tests, that would be a much better alternative. Orignally, I put the ASSERT at the site of the call as you suggest, but that did not make any sense once I applied the patch for checking the null renderer. IntRect FrameView::windowClipRect() ... ASSERT(elt && elt->renderer()); // Patch for null renderer RenderLayer* layer = 0; if (elt && elt->renderer()) layer = elt->renderer()->enclosingLayer(); So I thought the best place to include the ASSERT was in enclosingLayer(). I agree that the ASSERT is a bit odd. However, I could not think of a better way to expose this problem in the Layout tests. I am open to other suggestions. While this currently only fails on Symbian, I assume we don't want WebKit depending on undefined C++ behavior. I hope this makes sense. If not, please ask more questions.

(In reply to comment #23) > (In reply to comment #22) > Now I understand your question. > > I added this ASSERT so that this situation would fail on all platforms > so I could write the test. If we had Symbian layout tests, that would > be a much better alternative. > > Orignally, I put the ASSERT at the site of > the call as you suggest, but that did not make any sense once I applied > the patch for checking the null renderer. > > IntRect FrameView::windowClipRect() > ... > ASSERT(elt && elt->renderer()); > > // Patch for null renderer > RenderLayer* layer = 0; > if (elt && elt->renderer()) > layer = elt->renderer()->enclosingLayer(); > > So I thought the best place to include the ASSERT was in > enclosingLayer(). The first idea was better than the current one. One thing bother me. If the test is good enough you don't need the ASSERT, it should just crash properly therefore it will be catch at a LayoutTest failure. A crashing test is good enough to be treated as a regression/failure. > > I agree that the ASSERT is a bit odd. However, I could not think > of a better way to expose this problem in the Layout tests. I am > open to other suggestions. While this currently only fails on Symbian, > I assume we don't want WebKit depending on undefined C++ behavior. That's why I'm annoying you with my questions. We can't put such code. > > I hope this makes sense. If not, please ask more questions.

(In reply to comment #24) > (In reply to comment #23) > > (In reply to comment #22) > > Now I understand your question. > > > > I added this ASSERT so that this situation would fail on all platforms > > so I could write the test. If we had Symbian layout tests, that would > > be a much better alternative. > > > > Orignally, I put the ASSERT at the site of > > the call as you suggest, but that did not make any sense once I applied > > the patch for checking the null renderer. > > > > IntRect FrameView::windowClipRect() > > ... > > ASSERT(elt && elt->renderer()); > > > > // Patch for null renderer > > RenderLayer* layer = 0; > > if (elt && elt->renderer()) > > layer = elt->renderer()->enclosingLayer(); > > > > So I thought the best place to include the ASSERT was in > > enclosingLayer(). > > The first idea was better than the current one. > > One thing bother me. If the test is good enough you don't need the ASSERT, it should just crash properly therefore it will be catch at a LayoutTest failure. A crashing test is good enough to be treated as a regression/failure. The problem here, that RenderObject::enclosingLayer doesn't deference any data member if this is null: RenderLayer* RenderObject::enclosingLayer() const { const RenderObject* curr = this; while (curr) { But on symbian something is deferenced on function enter (IMHO, VMT if function has VM calls). So, this function is designed to work fine with this pointer null, but it doesn't work on symbian because of code generation specifics.

(In reply to comment #24) > > enclosingLayer(). > The first idea was better than the current one. > One thing bother me. If the test is good enough you don't need the ASSERT, it should just crash properly therefore it will be catch at a LayoutTest failure. A crashing test is good enough to be treated as a regression/failure. That's the problem. The test without the ASSERT is not good enough to crash on the Unix platforms. The Unix platforms allow a null "this" pointer and Symbian does not. I can't think of a way to make a better test.

(In reply to comment #25) > (In reply to comment #24) > > (In reply to comment #23) > > > (In reply to comment #22) > > > Now I understand your question. > > > > > > I added this ASSERT so that this situation would fail on all platforms > > > so I could write the test. If we had Symbian layout tests, that would > > > be a much better alternative. > > > > > > Orignally, I put the ASSERT at the site of > > > the call as you suggest, but that did not make any sense once I applied > > > the patch for checking the null renderer. > > > > > > IntRect FrameView::windowClipRect() > > > ... > > > ASSERT(elt && elt->renderer()); > > > > > > // Patch for null renderer > > > RenderLayer* layer = 0; > > > if (elt && elt->renderer()) > > > layer = elt->renderer()->enclosingLayer(); > > > > > > So I thought the best place to include the ASSERT was in > > > enclosingLayer(). > > > > The first idea was better than the current one. > > > > One thing bother me. If the test is good enough you don't need the ASSERT, it should just crash properly therefore it will be catch at a LayoutTest failure. A crashing test is good enough to be treated as a regression/failure. > > The problem here, that RenderObject::enclosingLayer doesn't deference any data member if this is null: > > RenderLayer* RenderObject::enclosingLayer() const > { > const RenderObject* curr = this; > while (curr) { > > But on symbian something is deferenced on function enter (IMHO, VMT if function has VM calls). So, this function is designed to work fine with this pointer null, but it doesn't work on symbian because of code generation specifics. But it doesn't matter if this function deference any data-member, you *should* never enter here from a pointer->enclosingLayer() where pointer is null. If I follow your logic we should put ASSERT(this) in *every* functions of WebKit, just in case we end up with a null this, this is wrong. This function was obviously NOT designed to work with a null this, it just goes from the current RenderObject (the this) and walk to the parents to find the first RenderLayer. The function enclosingBox is doing something similar.

(In reply to comment #26) > (In reply to comment #24) > > > enclosingLayer(). > > The first idea was better than the current one. > > One thing bother me. If the test is good enough you don't need the ASSERT, it should just crash properly therefore it will be catch at a LayoutTest failure. A crashing test is good enough to be treated as a regression/failure. > > That's the problem. The test without the ASSERT is not good enough > to crash on the Unix platforms. The Unix platforms allow a null > "this" pointer and Symbian does not. I can't think of a way to > make a better test. Either ASSERT earlier or make a specific layout test only executed on Symbian (look in LayoutTests/platform/qt, we have different sets of Skipped tests, e.g. one with Qt 4.8).

(In reply to comment #28) > (In reply to comment #26) > > (In reply to comment #24) > > > > enclosingLayer(). > > > The first idea was better than the current one. > > > One thing bother me. If the test is good enough you don't need the ASSERT, it should just crash properly therefore it will be catch at a LayoutTest failure. A crashing test is good enough to be treated as a regression/failure. > > > > That's the problem. The test without the ASSERT is not good enough > > to crash on the Unix platforms. The Unix platforms allow a null > > "this" pointer and Symbian does not. I can't think of a way to > > make a better test. > > Either ASSERT earlier or make a specific layout test only executed on Symbian (look in LayoutTests/platform/qt, we have different sets of Skipped tests, e.g. one with Qt 4.8). And it doesn't matter if you don't cover other platforms in your tests because anyway they didn't crash earlier. What you want to make sure is that Symbian is not crashing. So just let the test like it is, run it on all platforms (UNIX will never crash) but Symbian will (without the patch) and is covered if a regression could happen.

(In reply to comment #29) > And it doesn't matter if you don't cover other platforms in your tests because anyway they didn't crash earlier. What you want to make sure is that Symbian is not crashing. So just let the test like it is, run it on all platforms (UNIX will never crash) but Symbian will (without the patch) and is covered if a regression could happen. That makes sense. Thanks! So I really just need to remove the ASSERT.

Do you know what exactly on Symbian makes it different? I'm not sure what a compiler might want to do that makes calling a non-virtual function with a null "this" not work.

(In reply to comment #31) Good question. If I am reading the asm correctly, it looks like the compiler assumes "this != 0" and puts the loop check at the end of the loop. // comments added by me Built with ARM C/C++ Compiler, RVCT4.0 [Build 902] _ZNK7WebCore12RenderObject14enclosingLayerEv 0x000003e8: e5d0101a .... LDRB r1,[r0,#0x1a] 0x000003ec: e2011002 .... AND r1,r1,#2 0x000003f0: e1b010a1 .... LSRS r1,r1,#1 0x000003f4: 15901020 ... LDRNE r1,[r0,#0x20] 0x000003f8: e3510000 ..Q. CMP r1,#0 // if (layer) 0x000003fc: 11a00001 .... MOVNE r0,r1 0x00000400: 112fff1e ../. BXNE r14 0x00000404: e590000c .... LDR r0,[r0,#0xc] 0x00000408: e3500000 ..P. CMP r0,#0 // while (curr) 0x0000040c: 1afffff5 .... BNE {pc} - 0x24 ; 0x3e8 0x00000410: e12fff1e ../. BX r14 RenderLayer* RenderObject::enclosingLayer() const { const RenderObject* curr = this; while (curr) { RenderLayer* layer = curr->hasLayer() ? toRenderBoxModelObject(curr)->layer() : 0; if (layer) return layer; curr = curr->parent(); } return 0; }

I am still planning to work on this. I'm having some WebKit build problems on my linux box. As soon as I straighten that out, I'll resubmit a new patch.

Created attachment 97815 [details] Check for null renderer to avoid a reset. Removed the ASSERT(this). Now this test will only reset on platforms, like Symbian, that do not allow the "this" pointer of a nonvirtual function to be null.

Comment on attachment 97815 [details] Check for null renderer to avoid a reset. Forgot to set flags.

Comment on attachment 97815 [details] Check for null renderer to avoid a reset. View in context: https://bugs.webkit.org/attachment.cgi?id=97815&action=review > Source/WebCore/page/FrameView.cpp:2128 > + if (elt && elt->renderer()) > + layer = elt->renderer()->enclosingLayer(); Which of the added checks fails with the included test case? Please add a test case for the other one, or remove the check.

Comment on attachment 97815 [details] Check for null renderer to avoid a reset. View in context: https://bugs.webkit.org/attachment.cgi?id=97815&action=review > Source/WebCore/ChangeLog:8 > + Check for a null renderer to fix a reset. This should explain why the new check is needed. What is the website doing that's different from most pages?

Comment on attachment 97815 [details] Check for null renderer to avoid a reset. View in context: https://bugs.webkit.org/attachment.cgi?id=97815&action=review > Source/WebCore/ChangeLog:5 > + Without checking existence of the renderer of the element,tries to access the enclosing layer. Name of the bug should be updated.

(In reply to comment #36) I will change this to "if (elt->renderer())". I don't think elt can ever be null. > (From update of attachment 97815 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=97815&action=review > > > Source/WebCore/page/FrameView.cpp:2128 > > + if (elt && elt->renderer()) > > + layer = elt->renderer()->enclosingLayer(); > > Which of the added checks fails with the included test case? Please add a test case for the other one, or remove the check.

Some general comments about bug filing and patches: 1. The title of the bug should describe the user-visible symptom, not the fix. 2. The changelog should describe what the is, and what you did to fix it. For example, in the current case, why is elt->renderer() null here?

Created attachment 97845 [details] Check for a null renderer to avoid a crash. I removed the check for a null of elt and tried to better explain in the change log when the null renderer case arises.

Comment on attachment 97845 [details] Check for a null renderer to avoid a crash. View in context: https://bugs.webkit.org/attachment.cgi?id=97845&action=review > LayoutTests/ChangeLog:12 > + This test will only reset on platforms (like Symbian) that What does 'reset' mean here? Is it the same as crashing?

(In reply to comment #42) > (From update of attachment 97845 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=97845&action=review > > LayoutTests/ChangeLog:12 > > + This test will only reset on platforms (like Symbian) that > What does 'reset' mean here? Is it the same as crashing? Yes. It will dereference 0 and crash.

Comment on attachment 97845 [details] Check for a null renderer to avoid a crash. Channged "reset" to "crash" to be more clear.

Comment on attachment 97845 [details] Check for a null renderer to avoid a crash. View in context: https://bugs.webkit.org/attachment.cgi?id=97845&action=review > Source/WebCore/ChangeLog:8 > + Check for a null renderer to fix a reset. This situation can s/reset/crash > Source/WebCore/page/FrameView.cpp:2130 > + RenderLayer* layer = 0; > + // The renderer can sometimes be null when style="display:none" interacts > + // with external content and plugins. > + if (elt->renderer()) > + layer = elt->renderer()->enclosingLayer(); You could write this as layer = elt->renderer() ? elt->renderer()->enclosingLayer().

Comment on attachment 97845 [details] Check for a null renderer to avoid a crash. View in context: https://bugs.webkit.org/attachment.cgi?id=97845&action=review > LayoutTests/plugins/hidden-iframe-with-swf-plugin.html:42 > +If this test does not assert or reset it and the line below reads "PASSED", it passes. The test also needs to get rid of "reset".

Created attachment 98796 [details] Check for a null renderer to avoid a crash. Changed "reset" to "crash", and updated check for null renderer to use the ?: operator.

Comment on attachment 98796 [details] Check for a null renderer to avoid a crash. Clearing flags on attachment: 98796 Committed r89876: <http://trac.webkit.org/changeset/89876>

All reviewed patches have been landed. Closing bug.

Revision r89876 cherry-picked into qtwebkit-2.2 with commit dad7a55 <http://gitorious.org/webkit/qtwebkit/commit/dad7a55>