Bug 8739 - Crash in RenderTableSection::paint due to manipulating DOM on resize
: Crash in RenderTableSection::paint due to manipulating DOM on resize
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: HTML DOM
: 420+
: Macintosh Mac OS X 10.4
: P1 Normal
Assigned To: Nobody
http://dgl.cx/dump/2005/04/safari-cra...
: InRadar
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-05-04 13:30 PDT by David Leadbeater
Modified: 2006-05-15 01:34 PDT (History)
3 users (show)

See Also:


Attachments
Backtrace (5.69 KB, text/plain)
2006-05-04 13:47 PDT, mitz@webkit.org
no flags Details
Patch, including change log and manual test (3.15 KB, patch)
2006-05-12 15:05 PDT, mitz@webkit.org
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Leadbeater 2006-05-04 13:30:14 PDT
See referenced URLs.

Safari from 1.3 up to latest nightly webkit build crash on the above URLs which maniuplate the DOM on resize.

This test case was developed from a real application.
Comment 1 mitz@webkit.org 2006-05-04 13:47:33 PDT
Created attachment 8113 [details]
Backtrace
Comment 2 mitz@webkit.org 2006-05-12 15:05:37 PDT
Created attachment 8272 [details]
Patch, including change log and manual test
Comment 3 Eric Seidel 2006-05-12 15:21:29 PDT
Comment on attachment 8272 [details]
Patch, including change log and manual test

Darin shoudl take a look at this.

This is great news though!  This is the same backtrace as our most common (yet non-reproducible) crasher!
Comment 4 Darin Adler 2006-05-12 20:51:38 PDT
Comment on attachment 8272 [details]
Patch, including change log and manual test

I'm not sure this is the best fix. What code is relying on a layout having been done? Why doesn't that code trigger a layout if it's pending? An explicit layout here seems to just cover over the real problem.
Comment 5 mitz@webkit.org 2006-05-13 01:02:35 PDT
(In reply to comment #4)
> IWhat code is relying on a layout having been
> done?

The painting code is invoked by -[WebHTMLView _recursiveDisplayAllDirtyWithLockFocus:visRect:] after it calls -[WebHTMLView _web_layoutIfNeededRecursive:testDirtyRect:].

> Why doesn't that code trigger a layout if it's pending?

Calling _web_layoutIfNeededRecursive:testDirtyRect: is doing just that.

> An explicit
> layout here seems to just cover over the real problem.

I think "the real problem" here is that there is a method that promises to do layout but fails to deliver. That method is -[WebHTMLView layoutToMinimumPageWidth:maximumPageWidth:adjustingViewSize:], and the proposed patch ensures that it will always do what it says it does.
Comment 6 Dave Hyatt 2006-05-13 01:45:04 PDT
layoutToMinimumPageWidth calls layout though (through the forceLayout functions).  Then it sends the resize event (which may schedule another layout).  I guess I need a better understanding of why forceLayout didn't deliver.
Comment 7 mitz@webkit.org 2006-05-13 01:53:03 PDT
(In reply to comment #6)
> layoutToMinimumPageWidth calls layout though (through the forceLayout
> functions).  Then it sends the resize event (which may schedule another
> layout).  I guess I need a better understanding of why forceLayout didn't
> deliver.

As you said, it schedules another layout, but that doesn't happen until it's too late, since _recursiveDisplayAllDirtyWithLockFocus:visRect proceeds with painting right after it asks for layout. I really don't see what the mystery is :-\
Comment 8 mitz@webkit.org 2006-05-13 01:55:38 PDT
Perhaps I should clarify that the objects that don't have layout are those added by the resize event handler.
Comment 9 Dave Hyatt 2006-05-13 01:59:01 PDT
Ah, ok, yes, that was the clarification I needed.

With your patch, though, we do one layout, resize, and then do another layout.  Couldn't we do the resize check at the top of the function, and then just let the remaining code do the single layout?
Comment 10 mitz@webkit.org 2006-05-13 03:25:30 PDT
(In reply to comment #9)
> With your patch, though, we do one layout, resize, and then do another layout. 
> Couldn't we do the resize check at the top of the function, and then just let
> the remaining code do the single layout?

So we talked about this on IRC. It's true that this fix add a layout, but only in the resize-handler-that-manupulates-the-DOM case.

If you want to do just one layout, you need to go deep into Frame::forceLayoutWithPageWidthRange and separate it into a function that sets the width and marks as needing layout (which you will call into before sending the resize event) and a function that forces layout (possibly twice for printing?) (which you will call into after sending the resize event). I'm not sure it's worth it.
Comment 11 Darin Adler 2006-05-13 08:46:41 PDT
Comment on attachment 8272 [details]
Patch, including change log and manual test

OK. With the new explanation I think this is an acceptable way to fix the bug. The change log is not good, though. prepare-ChangeLog used the wrong class name!
Comment 12 Darin Adler 2006-05-14 21:32:56 PDT
Committed revision 14371.
Comment 13 Eric Seidel 2006-05-15 01:34:30 PDT
I'm hopeful this fixed:
<rdar://problem/4018768> unrepro but oft-reported Safari crash in khtml::RenderTableSection::paint