Bug 8739 – Crash in RenderTableSection::paint due to manipulating DOM on resize

• Component: DOM
• Status: RESOLVED FIXED

[David Leadbeater]

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.

Created attachment 8113 [details]
Backtrace

Created attachment 8272 [details]
Patch, including change log and manual test

[Eric Seidel (no email)]

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!

[Darin Adler]

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.

(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.

[Dave Hyatt]

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.

(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 :-\

Perhaps I should clarify that the objects that don't have layout are those added by the resize event handler.

[Dave Hyatt]

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?

(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.

[Darin Adler]

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!

[Darin Adler]

Committed revision 14371.

[Eric Seidel (no email)]

I'm hopeful this fixed:
<rdar://problem/4018768> unrepro but oft-reported Safari crash in khtml::RenderTableSection::paint