Bug 15241 – REGRESSION (r25124-r25140): Reproducible crash in WebCore::bidiNext inside NetNewsWire

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Mark Rowe (bdash)]

1. Launch NetNewsWire either on Tiger using a nightly build of WebKit, or on Leopard.
2. Subscribe to http://ejohn.org/blog/index.rdf.
3. Select the new subscription in the sidebar.
4. Click on the article titled "jQuery at the Boston Ruby Group".
5. *crash*

Several other articles on the same feed also trigger the crash.

<rdar://problem/5466459>

[Mark Rowe (bdash)]

Created attachment 16328 [details]
Crash log

[Mark Rowe (bdash)]

This happens with NetNewsWire 3.0, a trial version of which is available at http://www.newsgator.com/download/products/NetNewsWire3.0.dmg.zip.

[Mark Rowe (bdash)]

The URL in step 2 should be http://ejohn.org/index.rdf.

[Adele Peterson]

looks like its crashing at:
237	            if (!skipInlines && !oldEndOfInline && current->isInlineFlow()) {

9/19/07 4:33 PM Adele Peterson:
I think current might be already released.
(gdb) p *current
$2 = {
  <WebCore::CachedResourceClient> = {
    _vptr$CachedResourceClient = 0x6c
  }, 
  members of WebCore::RenderObject: 
  m_style = 0x1c691b7c, 
  m_node = 0x0, 
  m_parent = 0x0, 
  m_previous = 0x0, 
  m_next = 0x0, 
  m_verticalPosition = 8191, 
  m_needsLayout = false, 
  m_normalChildNeedsLayout = false, 
  m_posChildNeedsLayout = false, 
  m_prefWidthsDirty = true, 
  m_floating = false, 
  m_positioned = false, 
  m_relPositioned = false, 
  m_paintBackground = false, 
  m_isAnonymous = false, 
  m_isText = false, 
  m_inline = true, 
  m_replaced = true, 
  m_isDragging = false, 
  m_hasLayer = false, 
  m_hasOverflowClip = false, 
  m_hasOverrideSize = false, 
  m_hasCounterNodeMap = false

9/19/07 4:33 PM Adele Peterson:
btw, I used Mark's steps to repro.

[Adele Peterson]

cc'ing Mitz & Beth

I can reproduce the crash with the r25140 nightly but not with the r25124 nightly. I never managed to reproduce it with a debug build.

Created attachment 16330 [details]
Reduction (will crash Safari if plug-ins are disabled)

The reduction makes me think this bug was caused (or uncovered) by <http://trac.webkit.org/projects/webkit/changeset/25128>.

r25128 made it possible for RenderPartObject::updateWidget() to be called during layout. That function can call HTMLObjectElement::renderFallbackContent(), which does a detach/attach. That's a Very Bad Thing to do during layout.

[Beth Dakin]

Created attachment 16374 [details]
Implement widget update queue on FrameView to updateWidget() when layout is done

Here is a patch that fixes the original crash in NetNewsWire. It fixes the reduction too, except that the reduction still ASSERTS on Debug builds in FrameView::layout() with:

ASSERT(!root->needsLayout());

Interestingly, NetNewsWire does not ASSERT anywhere with this patch. So, the problem with this patch is that there is no test. I am not sure how to test it since this test asserts and involves turning off plug-ins, and I am not sure if that is something that DumpRenderTree can currently handle.

All of the layout tests pass with this patch.

[Darin Adler]

Comment on attachment 16374 [details]
Implement widget update queue on FrameView to updateWidget() when layout is done

Beth and I just discussed some refinements and she's going to write a new patch.

[Beth Dakin]

I have a new patch that addresses all of Darin's comments. However, I am trying to figure out the assertion failure before I post it because the failure does appear to be a regression. I reverted my tree to the revision before Anders' patch, and the reduction does not assert then. I will hopefully post a new patch soon.

[Beth Dakin]

Created attachment 16388 [details]
Addresses Darin's comments, fixes assertion.

This patch addresses all of Darin's comments from yesterday. I figured out that the assertion was firing because the reduction caused a nested layout to happen. That fact alone is kind of scary, but seems okay. The inner layout was running the updateWidget(), and when the stack popped back to the outer layout, the fact that updateWidget() had already been called meant that we needed layout and the assertion fired. I discussed this with Mitz and Geoff, and we decided that the best solution is to only call updateWidget from non-nested calls to FrameView::layout(). (Or outer-most calls, depending on how you look at it.) So that is what this patch does.

[Darin Adler]

Comment on attachment 16388 [details]
Addresses Darin's comments, fixes assertion.

+    ~RenderPartObject();

In cases like this I prefer to explicitly say "virtual" even though it's automatically virtual because the base class's destructor is virtual.

r=me

[Beth Dakin]

Committed fix http://trac.webkit.org/projects/webkit/changeset/25726