http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1271461515.1271462146.29061.gz WINNT 5.2 mozilla-central opt test mochitests-1/5 on 2010/04/16 16:45:15 s: win32-slave26 43872 INFO TEST-PASS | /tests/content/html/document/test/test_bug199692.html | Element from doubly nested iframe returned is from calling document 43873 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/document/test/test_bug199692.html | document.elementFromPoint not respecting scrolling? - got [object HTMLDivElement], expected [object HTMLDivElement] 43874 INFO TEST-PASS | /tests/content/html/document/test/test_bug199692.html | should have returned null for a not-visible point

So this is actually caused by lazy frame construction. We have <iframe src="file.html#id>, but the iframe never scrolls down to id some of the time, it is quite easy to create a simple testcase. This is the stack that normally does the scrolling. #0 PresShell::GoToAnchor (this=0x7f2e0aa67c00, aAnchorName=..., aScroll=1) at ../../../layout/base/nsPresShell.cpp:3556 #1 0x00007f2e277b134b in nsContentSink::ScrollToRef (this=0x7f2e0aa56000) at ../../../../content/base/src/nsContentSink.cpp:1245 #2 0x00007f2e278efa4c in HTMLContentSink::DidBuildModel (this=0x7f2e0aa56000, aTerminated=<value optimized out>) at ../../../../../content/html/document/src/nsHTMLContentSink.cpp:1798 #3 0x00007f2e2757309a in nsParser::DidBuildModel (this=0x7f2e0a916380, anErrorCode=<value optimized out>) at ../../../../parser/htmlparser/src/nsParser.cpp:1589 But when it doesn't work nsContentSink::ScrollToRef fails to do anything because the PresShell hasn't been created yet. It gets created when the frame for the iframe gets created. What I think we should do is move this ScrollToRef stuff from nsContentSink to nsDocument and then call ScrollToRef on the document in DocumentViewerImpl::InitPresentationStuff after we do an initial reflow (that seems to be the only place where we could do an initial reflow and fail to call ScrollToRef after).

Hmm. That would work, but we should be a little careful about what should happen if an iframe with a ref in the url is switched to display:none and then back. Should it scroll? What if the user had scrolled it before it became display:none? Maybe it's not an issue; we lose scroll positions in that situation anyway...

nsContentSink has a boolean that makes it only scroll to a ref once, we would have the same for when the document does the scroll to ref, so we wouldn't try scrolling to the ref when going from display: none to display: something. The user's scroll position gets restored even if we have a ref in the iframe url thanks to PresShell::RestoreRootScrollPosition and scroll frames being nsIStatefulFrame's (both before and after my suggested fix).

Comment on attachment 441630 [details] [diff] [review] proposed patch stealing review from bz. r=jst

Landed http://hg.mozilla.org/mozilla-central/rev/a6138098775f but backed out for causing failures http://hg.mozilla.org/mozilla-central/rev/357d30215706 http://hg.mozilla.org/mozilla-central/rev/b279d99d3f45

During the time when this landed, it caused a bunch of "should have done initial reflow by now" assertions on: layout/reftests/bugs/413292-1.html docshell/base/crashtests/436900-1.html editor/libeditor/base/crashtests/430624-1.html

The assert in PresShell::ScrollToAnchor was not correct in that ScrollToAnchor gets called at times before initial reflow and that is ok because ScrollToAnchor only tries to scroll if PresShell::GoToAnchor was previously called and set the anchor to scroll to. That's the main problem. docshell/base/crashtests/436900-1.html tickles some real problems, so I think the remaining asserts point out a real problem. I plan to checkin a slightly modified version of this crashtest and will file a bug on some other peculiarities this test causes.

Landed again with fixed asserts http://hg.mozilla.org/mozilla-central/rev/8cde1892b3d4 And landed the tweaked version of docshell/base/crashtests/436900-1.html http://hg.mozilla.org/mozilla-central/rev/47b20a425ea0

Filed bug 566159 for comment 20.

This introduced a new intermittent assertion on docshell/base/crashtests/436900-2.html in which we assert twice: ###!!! ASSERTION: should have done initial reflow by now: 'mDidInitialReflow', file e:/builds/moz2_slave/mozilla-central-win32-debug/build/layout/base/nsPresShell.cpp, line 4015 ###!!! ASSERTION: should have done initial reflow by now: 'mDidInitialReflow', file e:/builds/moz2_slave/mozilla-central-win32-debug/build/layout/base/nsPresShell.cpp, line 4043

I filed bug 566387 on the new assertion.