[Webkit-unassigned] [Bug 56393] Without checking existence of the renderer of the element, tries to access the enclosing layer.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=56393





--- Comment #27 from Alexis Menard <alexis.menard at openbossa.org>  2011-06-14 13:33:25 PST ---
(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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.