Bug 15241 - REGRESSION (r25124-r25140): Reproducible crash in WebCore::bidiNext inside NetNewsWire
Summary: REGRESSION (r25124-r25140): Reproducible crash in WebCore::bidiNext inside Ne...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 523.x (Safari 3)
Hardware: Macintosh OS X 10.4
: P1 Major
Assignee: Beth Dakin
URL:
Keywords: HasReduction, InRadar, Regression
Depends on:
Blocks:
 
Reported: 2007-09-19 15:15 PDT by Mark Rowe (bdash)
Modified: 2007-09-25 11:47 PDT (History)
3 users (show)

See Also:


Attachments
Crash log (20.25 KB, text/plain)
2007-09-19 15:16 PDT, Mark Rowe (bdash)
no flags Details
Reduction (will crash Safari if plug-ins are disabled) (391 bytes, text/html)
2007-09-19 17:57 PDT, mitz
no flags Details
Implement widget update queue on FrameView to updateWidget() when layout is done (5.83 KB, patch)
2007-09-24 16:09 PDT, Beth Dakin
darin: review-
Details | Formatted Diff | Diff
Addresses Darin's comments, fixes assertion. (6.37 KB, patch)
2007-09-25 11:26 PDT, Beth Dakin
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Rowe (bdash) 2007-09-19 15:15:51 PDT
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>
Comment 1 Mark Rowe (bdash) 2007-09-19 15:16:37 PDT
Created attachment 16328 [details]
Crash log
Comment 2 Mark Rowe (bdash) 2007-09-19 15:18:56 PDT
This happens with NetNewsWire 3.0, a trial version of which is available at http://www.newsgator.com/download/products/NetNewsWire3.0.dmg.zip.
Comment 3 Mark Rowe (bdash) 2007-09-19 15:21:18 PDT
The URL in step 2 should be http://ejohn.org/index.rdf.
Comment 4 Adele Peterson 2007-09-19 16:48:28 PDT
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.
Comment 5 Adele Peterson 2007-09-19 16:50:19 PDT
cc'ing Mitz & Beth
Comment 6 mitz 2007-09-19 17:38:48 PDT
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.
Comment 7 mitz 2007-09-19 17:57:43 PDT
Created attachment 16330 [details]
Reduction (will crash Safari if plug-ins are disabled)
Comment 8 mitz 2007-09-19 18:00:42 PDT
The reduction makes me think this bug was caused (or uncovered) by <http://trac.webkit.org/projects/webkit/changeset/25128>.
Comment 9 mitz 2007-09-19 18:26:52 PDT
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.
Comment 10 Beth Dakin 2007-09-24 16:09:34 PDT
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.
Comment 11 Darin Adler 2007-09-24 16:49:42 PDT
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.
Comment 12 Beth Dakin 2007-09-24 22:15:21 PDT
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.
Comment 13 Beth Dakin 2007-09-25 11:26:24 PDT
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.
Comment 14 Darin Adler 2007-09-25 11:35:04 PDT
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
Comment 15 Beth Dakin 2007-09-25 11:47:05 PDT
Committed fix http://trac.webkit.org/projects/webkit/changeset/25726