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
: WebKit
HTML DOM
: 420+
: Macintosh Mac OS X 10.4
: P1 Normal
Assigned To:
: http://dgl.cx/dump/2005/04/safari-cra...
: InRadar
:
:
  Show dependency treegraph
 
Reported: 2006-05-04 13:30 PST by
Modified: 2006-05-15 01:34 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2006-05-04 13:30:14 PST
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 From 2006-05-04 13:47:33 PST -------
Created an attachment (id=8113) [details]
Backtrace
------- Comment #2 From 2006-05-12 15:05:37 PST -------
Created an attachment (id=8272) [details]
Patch, including change log and manual test
------- Comment #3 From 2006-05-12 15:21:29 PST -------
(From update of attachment 8272 [details])
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 From 2006-05-12 20:51:38 PST -------
(From update of attachment 8272 [details])
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 From 2006-05-13 01:02:35 PST -------
(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 From 2006-05-13 01:45:04 PST -------
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 From 2006-05-13 01:53:03 PST -------
(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 From 2006-05-13 01:55:38 PST -------
Perhaps I should clarify that the objects that don't have layout are those added by the resize event handler.
------- Comment #9 From 2006-05-13 01:59:01 PST -------
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 From 2006-05-13 03:25:30 PST -------
(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 From 2006-05-13 08:46:41 PST -------
(From update of attachment 8272 [details])
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 From 2006-05-14 21:32:56 PST -------
Committed revision 14371.
------- Comment #13 From 2006-05-15 01:34:30 PST -------
I'm hopeful this fixed:
<rdar://problem/4018768> unrepro but oft-reported Safari crash in khtml::RenderTableSection::paint