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
Summary: REGRESSION: If setTimeout is called on a iframe's window, the DOM changes to ...
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P1 Normal
Assignee: Dmitry Titov
URL: http://ludios.net/safari4_dom_flush_b...
Keywords: InRadar, Regression
Depends on:
Reported: 2009-03-08 07:38 PDT by ivank
Modified: 2009-04-13 18:07 PDT (History)
4 users (show)

See Also:

test case (1.28 KB, text/html)
2009-03-09 10:09 PDT, Alexey Proskuryakov
no flags Details
test case (no onload) (1.25 KB, text/html)
2009-03-10 17:54 PDT, ivank
no flags Details
test case (no onload) fixed (1.32 KB, text/html)
2009-03-10 17:58 PDT, ivank
no flags Details
Proposed patch (1.76 KB, patch)
2009-03-11 16:09 PDT, Dmitry Titov
no flags Details | Formatted Diff | Diff
Repro file - can be loaded from disk. (1.03 KB, text/html)
2009-03-11 19:46 PDT, Dmitry Titov
no flags Details
Automated regression test. (17.36 KB, patch)
2009-03-15 00:01 PDT, Dmitry Titov
no flags Details | Formatted Diff | Diff
Proposed patch (2.63 KB, patch)
2009-04-13 12:49 PDT, Dmitry Titov
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description ivank 2009-03-08 07:38:01 PDT
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.
Comment 1 ivank 2009-03-08 08:04:13 PDT
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.

Comment 2 Alexey Proskuryakov 2009-03-09 10:09:36 PDT
Created attachment 28416 [details]
test case

bugs.webkit.org uses https, copying here.
Comment 3 Alexey Proskuryakov 2009-03-09 10:12:55 PDT
Confirmed as a regression (test passes on Mac on 3.2.1, fails on a local debug ToT build).
Comment 4 Alexey Proskuryakov 2009-03-09 10:14:25 PDT
It's quite puzzling that there is a difference between http and https, but maybe it's just timing.
Comment 5 Dmitry Titov 2009-03-10 14:45:20 PDT
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...
Comment 6 Dmitry Titov 2009-03-10 14:46:35 PDT
And, previous nightlies do not run in 3.2.1 anymore so hard to pinpoint the change.

Comment 7 ivank 2009-03-10 17:54:42 PDT
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.
Comment 8 ivank 2009-03-10 17:57:01 PDT
Attached wrong test case, hold on.
Comment 9 ivank 2009-03-10 17:58:15 PDT
Created attachment 28457 [details]
test case (no onload) fixed

Proper test case for "test case (no onload)"
Comment 10 Dmitry Titov 2009-03-11 16:09:37 PDT
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.
Comment 11 Dmitry Titov 2009-03-11 19:46:31 PDT
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.
Comment 12 Dmitry Titov 2009-03-11 20:57:57 PDT
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.
Comment 13 Alexey Proskuryakov 2009-03-12 03:10:41 PDT
CC'ing Mitz, as he's the expert on making such tests, and might have a suggestion.
Comment 14 Dmitry Titov 2009-03-12 15:35:37 PDT
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.
Comment 15 Dmitry Titov 2009-03-15 00:01:52 PDT
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.
Comment 16 Dmitry Titov 2009-04-13 10:54:40 PDT
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.
Comment 17 Alexey Proskuryakov 2009-04-13 11:13:56 PDT
Comment 18 Dmitry Titov 2009-04-13 12:22:04 PDT
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.
Comment 19 Dmitry Titov 2009-04-13 12:49:59 PDT
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.
Comment 20 Darin Adler 2009-04-13 16:59:30 PDT
Comment on attachment 29434 [details]
Proposed patch

Comment 21 Dmitry Titov 2009-04-13 18:07:55 PDT
Landed: http://trac.webkit.org/changeset/42474