Phil Ringnalda (:philor) Reporter
	Description • 14 years ago

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

	Timothy Nikkel (:tnikkel) Assignee
	Comment 5 • 14 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 6 • 14 years ago

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

	Timothy Nikkel (:tnikkel) Assignee
	Comment 7 • 14 years ago

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

	Johnny Stenback (:jst)
	Comment 13 • 14 years ago

Comment on attachment 441630 [details] [diff] [review]
proposed patch

stealing review from bz. r=jst

	Timothy Nikkel (:tnikkel) Assignee
	Comment 16 • 14 years ago

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

	David Baron :dbaron:
	Comment 17 • 14 years ago

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

	Timothy Nikkel (:tnikkel) Assignee
	Comment 20 • 14 years ago

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.

	Timothy Nikkel (:tnikkel) Assignee
	Comment 21 • 14 years ago

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

	Timothy Nikkel (:tnikkel) Assignee
	Comment 22 • 14 years ago

Filed bug 566159 for comment 20.

	David Baron :dbaron:
	Comment 23 • 14 years ago

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

	David Baron :dbaron:
	Comment 24 • 14 years ago

I filed bug 566387 on the new assertion.