Bug 24453 – REGRESSION: If setTimeout is called on a iframe's window, the DOM changes to the main document that timer callback makes are not flushed

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[ivank]

See the URL for a reproduction of the bug.

On Safari 4/XP SP3, using a frame's setTimeout to call a callable that modifies the DOM may leave those changes unflushed until a variety of things happen:

user moves the mouse or clicks
user presses a key
user has Error Console open

The expected behavior is for the changes to be flushed near-immediately.

[ivank]

This bug is NOT reproducible when the page is loaded over http:// (only https://).

The linked-to URL will not reproduce the bug, unless the page is copied to an https:// server.

[Alexey Proskuryakov]

Created attachment 28416 [details]
test case

bugs.webkit.org uses https, copying here.

[Alexey Proskuryakov]

Confirmed as a regression (test passes on Mac on 3.2.1, fails on a local debug ToT build).

[Alexey Proskuryakov]

It's quite puzzling that there is a difference between http and https, but maybe it's just timing.

[Dmitry Titov]

Reproducible in all nightlies starting r37442	(2008-10-09). 
I think the issue here is with using onload event on iframe - apparently, after adding <HR> to the enclosing document, the timer callback invalidates the iframe's document.
Looking for a fix...

[Dmitry Titov]

And, previous nightlies do not run in 3.2.1 anymore so hard to pinpoint the change.

[ivank]

Created attachment 28456 [details]
test case (no onload)

This test case does not use onload on the iframe, but behaves just like the last one.

[ivank]

Attached wrong test case, hold on.

[ivank]

Created attachment 28457 [details]
test case (no onload) fixed

Proper test case for "test case (no onload)"

[Dmitry Titov]

Created attachment 28500 [details]
Proposed patch

The key here is that setTimeout is called on a window object of <iframe>. This creates a timer that has that window as an 'executing context'. The callback's JS function is defined in the parent document's scope and when it executes, it updates the parent document. However, the code calls updateRendering() on a document that is associated with the window of <iframe> - and with certain timing, it causes missing update of the parent document until some other event (like mousemove) causes update.

The proposed fix is to use Document::updateDocumentsRendering() instead since it will update all the affected documents right away.

So far, I was unable to create a local test that would reproduce the bug.

[Dmitry Titov]

Created attachment 28517 [details]
Repro file - can be loaded from disk.

Found a way to repro this from locally-saved file. No server needed.
Just need to wait for >1sec.
Will post updated patch, with a regression test.

[Dmitry Titov]

Comment on attachment 28500 [details]
Proposed patch

Actually, I don't see how to make automated test out of it. DRT causes layout update when it tries to access any computed property or make an image or render tree dump. Flipping r? back on the patch w/o test.

[Alexey Proskuryakov]

CC'ing Mitz, as he's the expert on making such tests, and might have a suggestion.

[Dmitry Titov]

More info: I've tried to create a DRT-based test, but it seems any attempt to 'snapshot' the result causes one extra FrameView::layout() one way or another and as a result, I get a correct snapshot even without the fix.

The buggy behavior here is that the setTimer callback not always invalidates layout of the right frame so the picture does not update until something else triggers layout. Lot of things trigger layout - among them externalRepresentation() function used to produce dump in DRT. I've tried to get a bitmap dump first but then I get ASSERTS because rendering of dirty layout is not a good idea.

I think exposing FrameView::needsLayout() to be readable on LayoutTestController could be one way to enable such tests - make some changes, then check if layout was in fact invalidated.

Not sure though if this is the easiest way.

[Dmitry Titov]

Created attachment 28633 [details]
Automated regression test.

This is one way to create a regression test for this bug. We can expose a new method on layoutTestController that reports invalidated layout. Here is the change, the test for the new layoutTestController method and a test for this bug. 
If this seems like a good solution, then this should go in first and the fix with updated expected results second.

[Dmitry Titov]

Comment on attachment 28633 [details]
Automated regression test.

Removing r? from the test since it feels weird to expose internal flags for the test purposes. I don't know how to make a test for this bug.

[Alexey Proskuryakov]

<rdar://problem/6784969>

[Dmitry Titov]

Changed the title to match the actual issue. I believe http://trac.webkit.org/changeset/42384 also fixes this bug in a different way, but need to verify this.

[Dmitry Titov]

Created attachment 29434 [details]
Proposed patch

Updated proposed fix after recent changes. Although style recalc timer catches the update, so the updateStyleForAllDocuments() call may be removed at all now, doing so may produce other regressions.

[Darin Adler]

Comment on attachment 29434 [details]
Proposed patch

r=me

[Dmitry Titov]

Landed: http://trac.webkit.org/changeset/42474