Intermittent failure in test_bug199692.html | document.elementFromPoint not respecting scrolling? - got [object HTMLDivElement], expected [object HTMLDivElement]

RESOLVED FIXED in mozilla1.9.3a5

Status

()

defect
RESOLVED FIXED
9 years ago
4 months ago

People

(Reporter: philor, Assigned: tnikkel)

Tracking

(Depends on 3 bugs, {intermittent-failure})

Trunk
mozilla1.9.3a5
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 final+)

Details

Attachments

(1 attachment)

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
Blocks: lazyfc
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).
Attachment #441630 - Flags: review?(bzbarsky)
Comment on attachment 441630 [details] [diff] [review]
proposed patch

stealing review from bz. r=jst
Attachment #441630 - Flags: review?(bzbarsky) → review+
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: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
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.
Depends on: 566387
Target Milestone: --- → mozilla1.9.3a5
Blocks: 586613
blocking2.0: ? → final+
Depends on: 645075
Depends on: 809461
Whiteboard: [orange]
Depends on: 911122
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.