Closed Bug 559996 Opened 11 years ago Closed 11 years ago
Intermittent failure in test
_bug199692 .html | document .element From Point not respecting scrolling? - got [object HTMLDiv Element], expected [object HTMLDiv Element]
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).
Assignee: nobody → tnikkel
blocking2.0: --- → ?
OS: Windows Server 2003 → All
Hardware: x86 → All
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
Attachment #441630 - Flags: review?(bzbarsky) → review+
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
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
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
You need to log in before you can comment on or make changes to this bug.