Bug 56393

Summary: Crash on www.crave.cnet.com in FrameView::windowClipRect()
Product: WebKit Reporter: suchi <suchi.kundu>
Component: Layout and RenderingAssignee: Joe Wild <joseph.wild>
Status: RESOLVED FIXED    
Severity: Major CC: ademar, ap, benjamin, cmarcelo, cmarrin, eric, joel.parks, joseph.wild, laszlo.gombos, menard, mitz, ostap73, simon.fraser, webkit.review.bot
Priority: P1 Keywords: Qt, QtTriaged
Version: 528+ (Nightly build)   
Hardware: S60 Hardware   
OS: S60 3rd edition   
Bug Depends on:    
Bug Blocks: 32653    
Attachments:
Description Flags
Check the existence of the renderer of element, before trying to access the layer.
eric: review-
Modified changelog
benjamin: review-
Example Layout test for this problem
none
FrameView null renderer fix for WebKit 2.2
none
Check for null renderer to avoid reset.
none
Check for null renderer to avoid a reset.
none
Check for a null renderer to avoid a crash.
simon.fraser: review+, simon.fraser: commit-queue-
Check for a null renderer to avoid a crash. none

Description suchi 2011-03-15 11:40:42 PDT
In function FrameView::windowClipRect(bool clipToContents)  we have following code where we try to access enclosing layer without checking the existence of renderer, which cause crashes in some circumstances.


// Take our owner element and get the clip rect from the enclosing layer.

     Element* elt = m_frame->document()->ownerElement();
     RenderLayer* layer = elt->renderer()->enclosingLayer();
Comment 1 suchi 2011-03-16 07:56:25 PDT
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 2 Eric Seidel 2011-03-17 04:21:17 PDT
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 3 Eric Seidel 2011-03-17 04:21:38 PDT
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/
Comment 4 Eric Seidel 2011-04-11 06:04:18 PDT
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?
Comment 5 suchi 2011-04-11 06:29:37 PDT
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.
Comment 6 suchi 2011-04-11 08:57:07 PDT
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.
Comment 7 suchi 2011-04-12 03:39:24 PDT
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 8 Benjamin Poulain 2011-04-12 04:44:16 PDT
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.
Comment 9 Viatcheslav Ostapenko 2011-04-26 09:29:15 PDT
(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?
Comment 10 Ademar Reis 2011-04-27 11:11:17 PDT
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.
Comment 11 Ademar Reis 2011-04-29 07:32:24 PDT
Patch added to 2.1 as c56e63a70e7186cd056b192f8a0056f6ed3bc84d, pending trunk inclusion and blocking 2.2.

P1 because it's a crash.
Comment 12 Joe Wild 2011-05-09 13:31:33 PDT
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.
Comment 13 Joe Wild 2011-05-19 07:24:35 PDT
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:
Comment 14 Eric Seidel 2011-05-29 14:06:36 PDT
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).
Comment 15 Joe Wild 2011-06-06 12:58:35 PDT
(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.
Comment 16 Joe Wild 2011-06-07 13:05:03 PDT
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.
Comment 17 Ademar Reis 2011-06-07 13:20:51 PDT
(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? :-(
Comment 18 Joe Wild 2011-06-08 15:06:07 PDT
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.
Comment 19 Joe Wild 2011-06-09 10:41:17 PDT
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 20 Alexis Menard (darktears) 2011-06-13 06:58:04 PDT
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.
Comment 21 Joe Wild 2011-06-14 10:27:38 PDT
(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.
Comment 22 Alexis Menard (darktears) 2011-06-14 11:01:42 PDT
(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.
Comment 23 Joe Wild 2011-06-14 12:18:01 PDT
(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.
Comment 24 Alexis Menard (darktears) 2011-06-14 12:35:52 PDT
(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.
Comment 25 Viatcheslav Ostapenko 2011-06-14 13:15:17 PDT
(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.
Comment 26 Joe Wild 2011-06-14 13:30:42 PDT
(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.
Comment 27 Alexis Menard (darktears) 2011-06-14 13:33:25 PDT
(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.
Comment 28 Alexis Menard (darktears) 2011-06-14 13:36:49 PDT
(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).
Comment 29 Alexis Menard (darktears) 2011-06-14 13:40:30 PDT
(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.
Comment 30 Joe Wild 2011-06-14 13:55:52 PDT
(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.
Comment 31 Alexey Proskuryakov 2011-06-14 14:05:13 PDT
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.
Comment 32 Joe Wild 2011-06-15 08:44:14 PDT
(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;
}
Comment 33 Joe Wild 2011-06-16 15:02:26 PDT
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.
Comment 34 Joe Wild 2011-06-20 09:52:19 PDT
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 35 Joe Wild 2011-06-20 10:02:46 PDT
Comment on attachment 97815 [details]
Check for null renderer to avoid a reset.

Forgot to set flags.
Comment 36 Alexey Proskuryakov 2011-06-20 10:07:09 PDT
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 37 Simon Fraser (smfr) 2011-06-20 10:12:37 PDT
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 38 Alexis Menard (darktears) 2011-06-20 10:16:32 PDT
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.
Comment 39 Joe Wild 2011-06-20 10:35:51 PDT
(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.
Comment 40 Simon Fraser (smfr) 2011-06-20 10:41:16 PDT
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?
Comment 41 Joe Wild 2011-06-20 13:12:26 PDT
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 42 Simon Fraser (smfr) 2011-06-20 13:23:42 PDT
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?
Comment 43 Joe Wild 2011-06-20 13:30:00 PDT
(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 44 Joe Wild 2011-06-27 13:34:28 PDT
Comment on attachment 97845 [details]
Check for a null renderer to avoid a crash.

Channged "reset" to "crash" to be more clear.
Comment 45 Simon Fraser (smfr) 2011-06-27 13:43:13 PDT
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 46 Alexey Proskuryakov 2011-06-27 13:49:33 PDT
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".
Comment 47 Joe Wild 2011-06-27 15:03:38 PDT
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 48 WebKit Review Bot 2011-06-27 16:26:31 PDT
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>
Comment 49 WebKit Review Bot 2011-06-27 16:26:41 PDT
All reviewed patches have been landed.  Closing bug.
Comment 50 Ademar Reis 2011-06-28 07:48:29 PDT
Revision r89876 cherry-picked into qtwebkit-2.2 with commit dad7a55 <http://gitorious.org/webkit/qtwebkit/commit/dad7a55>