Bug 102765

Created attachment 175136 [details] Patch

Comment on attachment 175136 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175136&action=review > Source/WebCore/dom/NodeRenderingContext.cpp:255 > + ASSERT(m_context.parentRenderer()); Can you move this line to be above the if/else. It makes more sense to me to assert before the line that depends on the assert.

Actually, even better, but it directly in the else statement.

I was about to r- because I think the ASSERTs should be positioned higher in the code flow, but Ojan beat me to it. If m_context.parentRenderer() might ever ASSERT, then it's possible the new code will blow up before having a chance to assert. I also had a question about the behavior of parentRenderer(), which I think is probably NOT a problem, but I'd feel better if you confirmed it. > Source/WebCore/ChangeLog:10 > + so there's no reason to check for it again. Is it possible for parentRenderer() to return null, but parentFlowRenderer to be non-null? From a brief look it seems like the answer is no. But if I'm wrong, this change would modify the behavior. Can you please confirm that this is not a problem?

(In reply to comment #3) > Actually, even better, but it directly in the else statement. I'm not actually asserting about the line in the else, I'm asserting about the rendererIsNeeded() on the following line after the assert. Calling that method on a context that doesn't have a style or a parentRenderer is always a mistake. Also if I put it in the else it makes it look like if isElementNode() is true and we don't go down the else that we may not have a renderer (which is never true). Also if we do go through that else we'll crash on the null dereference so there's no reason to ASSERT about it.

(In reply to comment #4) > I was about to r- because I think the ASSERTs should be positioned higher in the code flow, but Ojan beat me to it. > > If m_context.parentRenderer() might ever ASSERT, then it's possible the new code will blow up before having a chance to assert. Yes, that was intended. We do this all over webkit, and it's even been requested by Apple on my patches before that I NOT assert about nulls and instead just let the program crash the null dereference. > > I also had a question about the behavior of parentRenderer(), which I think is probably NOT a problem, but I'd feel better if you confirmed it. > > > Source/WebCore/ChangeLog:10 > > + so there's no reason to check for it again. > > Is it possible for parentRenderer() to return null, but parentFlowRenderer to be non-null? From a brief look it seems like the answer is no. But if I'm wrong, this change would modify the behavior. Can you please confirm that this is not a problem? I don't understand what you're asking. parentRenderer never returns null in this code path because shouldCreateRenderer would have returned before we got here. There's no behavior change.

Created attachment 175138 [details] Patch for landing

(In reply to comment #7) > Created an attachment (id=175138) [details] > Patch for landing I moved the assert bove the if/else.

(In reply to comment #6) > (In reply to comment #4) > > Is it possible for parentRenderer() to return null, but parentFlowRenderer to be non-null? From a brief look it seems like the answer is no. But if I'm wrong, this change would modify the behavior. Can you please confirm that this is not a problem? > > I don't understand what you're asking. parentRenderer never returns null in this code path because shouldCreateRenderer would have returned before we got here. There's no behavior change. I actually went and looked at the routine, and see that you are correct. Thanks!

Comment on attachment 175138 [details] Patch for landing Clearing flags on attachment: 175138 Committed r135252: <http://trac.webkit.org/changeset/135252>

All reviewed patches have been landed. Closing bug.