Bug 15241

Summary: REGRESSION (r25124-r25140): Reproducible crash in WebCore::bidiNext inside NetNewsWire
Product: WebKit Reporter: Mark Rowe (bdash) <mrowe>
Component: Layout and RenderingAssignee: Beth Dakin <bdakin>
Status: RESOLVED FIXED    
Severity: Major CC: andersca, bdakin, mitz
Priority: P1 Keywords: HasReduction, InRadar, Regression
Version: 523.x (Safari 3)   
Hardware: Mac   
OS: OS X 10.4   
Attachments:
Description Flags
Crash log
none
Reduction (will crash Safari if plug-ins are disabled)
none
Implement widget update queue on FrameView to updateWidget() when layout is done
darin: review-
Addresses Darin's comments, fixes assertion. darin: review+

Mark Rowe (bdash)
Reported 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>
Attachments
Crash log (20.25 KB, text/plain)
2007-09-19 15:16 PDT, Mark Rowe (bdash)
no flags
Reduction (will crash Safari if plug-ins are disabled) (391 bytes, text/html)
2007-09-19 17:57 PDT, mitz
no flags
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-
Addresses Darin's comments, fixes assertion. (6.37 KB, patch)
2007-09-25 11:26 PDT, Beth Dakin
darin: review+
Mark Rowe (bdash)
Comment 1 2007-09-19 15:16:37 PDT
Created attachment 16328 [details] Crash log
Mark Rowe (bdash)
Comment 2 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.
Mark Rowe (bdash)
Comment 3 2007-09-19 15:21:18 PDT
The URL in step 2 should be http://ejohn.org/index.rdf.
Adele Peterson
Comment 4 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.
Adele Peterson
Comment 5 2007-09-19 16:50:19 PDT
cc'ing Mitz & Beth
mitz
Comment 6 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.
mitz
Comment 7 2007-09-19 17:57:43 PDT
Created attachment 16330 [details] Reduction (will crash Safari if plug-ins are disabled)
mitz
Comment 8 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>.
mitz
Comment 9 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.
Beth Dakin
Comment 10 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.
Darin Adler
Comment 11 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.
Beth Dakin
Comment 12 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.
Beth Dakin
Comment 13 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.
Darin Adler
Comment 14 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
Beth Dakin
Comment 15 2007-09-25 11:47:05 PDT
Note You need to log in before you can comment on or make changes to this bug.