Closed Bug 926155 Opened 11 years ago Closed 10 years ago

Images can not be displayed when using the overflow-x: hidden; and position: sticky;

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: nayinain, Assigned: kip, NeedInfo)

References

Details

(Keywords: regression)

Attachments

(4 files, 5 obsolete files)

Attached file test.html
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:27.0) Gecko/20100101 Firefox/27.0 (Beta/Release)
Build ID: 20131012030202

Steps to reproduce:

1. Clear your FX's cache.
2. Open the html file.
3. Resize the FX's window.


Actual results:

Images displayed in step 3, not displayed in step 2.


Expected results:

In the step 2 and 3, Images should be displayed.



Sorry for my bad English.
Component: Untriaged → DOM: Core & HTML
Product: Firefox → Core
Component: DOM: Core & HTML → DOM: CSS Object Model
Component: DOM: CSS Object Model → Layout
Can reproduce in Nightly on Linux and OS X using Shift-Refresh.

I could imagine some sort of problem with the assumption, in the case of replaced elements, that all sizes have been figured out by the time we reflow the scroll container... though once we do figure their sizes out, I assume we'd have to reflow again anyway. And that wouldn't explain why the images themselves get 0x0 frames.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 7 → All
Hardware: x86_64 → All
Just making a note in case it helps - adding position: overflow to a parent container seems to fix the issue for me.
Version: 27 Branch → Trunk
(In reply to Corey Ford [:corey] from comment #1)
> Can reproduce in Nightly on Linux and OS X using Shift-Refresh.

Yes, I also see it only with Shift-reload (Shift+Ctrl+R).


This seems similar to bug 976655, and might be related.
Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/38d8c6c2c223
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:27.0) Gecko/20100101 Firefox/27.0 ID:20130918101519
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/2d033c301dfd
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:27.0) Gecko/20100101 Firefox/27.0 ID:20130918104117
Pushlog
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=38d8c6c2c223&tochange=2d033c301dfd

Triggered by:
2d033c301dfd	Corey Ford — Bug 902992 - Enable position:sticky in non-release builds (e.g. Nightly and Aurora). r=dholbert


Regression window w/ force layout.css.sticky.enabled = true (m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/9fa7a3f9e4d6
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 ID:20130906063454
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/bb399c38e3b6
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 ID:20130906063854
Pushlog
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=9fa7a3f9e4d6&tochange=bb399c38e3b6

Regressed by: Bug 886646
Blocks: 886646, 902992
Keywords: regression
An assertion fails every time the images fail to load:

ASSERTION: can't mark frame dirty during reflow: '!mIsReflowing', file /Users/kgilbert/dev/mozilla-central/layout/base/nsPresShell.cpp, line 2617

The call stack involves StickyScrollContainer::UpdatePositions
Stack trace for assert:

* thread #1: tid = 0x260141, 0x0000000103d673f3 XUL`PresShell::FrameNeedsReflow(this=0x0000000138fe94a0, aFrame=0x000000011b329c70, aIntrinsicDirty=eResize, aBitToAdd=NS_FRAME_HAS_DIRTY_CHILDREN) + 195 at nsPresShell.cpp:2617, queue = 'com.apple.main-thread, stop reason = breakpoint 1.1
    frame #0: 0x0000000103d673f3 XUL`PresShell::FrameNeedsReflow(this=0x0000000138fe94a0, aFrame=0x000000011b329c70, aIntrinsicDirty=eResize, aBitToAdd=NS_FRAME_HAS_DIRTY_CHILDREN) + 195 at nsPresShell.cpp:2617
    frame #1: 0x0000000103f066eb XUL`mozilla::ScrollFrameHelper::UpdateOverflow(this=0x000000011b329cf8) + 219 at nsGfxScrollFrame.cpp:4002
    frame #2: 0x0000000103f2699c XUL`nsHTMLScrollFrame::UpdateOverflow(this=0x000000011b329c70) + 28 at nsGfxScrollFrame.h:488
    frame #3: 0x0000000103c621ac XUL`mozilla::OverflowChangedTracker::Flush(this=0x00007fff5fbfba28) + 332 at RestyleTracker.h:116
    frame #4: 0x0000000103e7b394 XUL`mozilla::StickyScrollContainer::UpdatePositions(this=0x000000013cb752b0, aScrollPosition=0x00007fff5fbfba98, aSubtreeRoot=0x0000000112d3ad88) + 420 at StickyScrollContainer.cpp:384
    frame #5: 0x0000000103efcdc3 XUL`mozilla::ScrollFrameHelper::UpdateSticky(this=0x0000000112d3ae10) + 147 at nsGfxScrollFrame.cpp:4020
    frame #6: 0x0000000103efbcef XUL`nsHTMLScrollFrame::Reflow(this=0x0000000112d3ad88, aPresContext=0x0000000112cbea00, aDesiredSize=0x00007fff5fbfc088, aReflowState=0x00007fff5fbfbf88, aStatus=0x00007fff5fbfc334) + 2543 at nsGfxScrollFrame.cpp:863
    frame #7: 0x0000000103ea72c3 XUL`nsContainerFrame::ReflowChild(this=0x0000000112d3a058, aKidFrame=0x0000000112d3ad88, aPresContext=0x0000000112cbea00, aDesiredSize=0x00007fff5fbfc088, aReflowState=0x00007fff5fbfbf88, aX=0, aY=0, aFlags=0, aStatus=0x00007fff5fbfc334, aTracker=0x0000000000000000) + 323 at nsContainerFrame.cpp:958
    frame #8: 0x0000000103f7b8a4 XUL`ViewportFrame::Reflow(this=0x0000000112d3a058, aPresContext=0x0000000112cbea00, aDesiredSize=0x00007fff5fbfc2e0, aReflowState=0x00007fff5fbfc378, aStatus=0x00007fff5fbfc334) + 708 at nsViewportFrame.cpp:221
    frame #9: 0x0000000103d646d8 XUL`PresShell::DoReflow(this=0x0000000138fe94a0, target=0x0000000112d3a058, aInterruptible=true) + 2088 at nsPresShell.cpp:8283
    frame #10: 0x0000000103d6ccd0 XUL`PresShell::ProcessReflowCommands(this=0x0000000138fe94a0, aInterruptible=true) + 512 at nsPresShell.cpp:8439
    frame #11: 0x0000000103d6c8d8 XUL`PresShell::FlushPendingNotifications(this=0x0000000138fe94a0, aFlush=ChangesToFlush at 0x00007fff5fbfc7b8) + 1736 at nsPresShell.cpp:4091
    frame #12: 0x0000000103d8fbab XUL`nsRefreshDriver::Tick(this=0x000000013b939140, aNowEpoch=1394146536433875, aNowTime=TimeStamp at 0x00007fff5fbfcb80) + 2907 at nsRefreshDriver.cpp:1162
    frame #13: 0x0000000103d952cc XUL`mozilla::RefreshDriverTimer::TickDriver(driver=0x000000013b939140, jsnow=1394146536433875, now=TimeStamp at 0x00007fff5fbfcbb8) + 92 at nsRefreshDriver.cpp:168
    frame #14: 0x0000000103d95154 XUL`mozilla::RefreshDriverTimer::Tick(this=0x00000001001b06e0) + 308 at nsRefreshDriver.cpp:160
    frame #15: 0x0000000103d95011 XUL`mozilla::RefreshDriverTimer::TimerTick(aTimer=0x00000001001b0dc0, aClosure=0x00000001001b06e0) + 33 at nsRefreshDriver.cpp:185
    frame #16: 0x000000010113ca35 XUL`nsTimerImpl::Fire(this=0x00000001001b0dc0) + 981 at nsTimerImpl.cpp:551
    frame #17: 0x000000010113ce41 XUL`nsTimerEvent::Run(this=0x00000001129d3410) + 209 at nsTimerImpl.cpp:635
    frame #18: 0x0000000101137ca9 XUL`nsThread::ProcessNextEvent(this=0x00000001107055c0, mayWait=false, result=0x00007fff5fbfcf73) + 1561 at nsThread.cpp:643
    frame #19: 0x000000010103582b XUL`NS_ProcessPendingEvents(thread=0x00000001107055c0, timeout=20) + 171 at nsThreadUtils.cpp:210
    frame #20: 0x0000000102bbeee9 XUL`nsBaseAppShell::NativeEventCallback(this=0x0000000100159c30) + 201 at nsBaseAppShell.cpp:98
    frame #21: 0x0000000102b4abec XUL`nsAppShell::ProcessGeckoEvents(aInfo=0x0000000100159c30) + 428 at nsAppShell.mm:388
    frame #22: 0x00007fff911f4731 CoreFoundation`__CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17
    frame #23: 0x00007fff911e5ea2 CoreFoundation`__CFRunLoopDoSources0 + 242
    frame #24: 0x00007fff911e562f CoreFoundation`__CFRunLoopRun + 831
    frame #25: 0x00007fff911e50b5 CoreFoundation`CFRunLoopRunSpecific + 309
    frame #26: 0x00007fff85363a0d HIToolbox`RunCurrentEventLoopInMode + 226
    frame #27: 0x00007fff85363685 HIToolbox`ReceiveNextEventCommon + 173
    frame #28: 0x00007fff853635bc HIToolbox`_BlockUntilNextEventMatchingListInModeWithFilter + 65
    frame #29: 0x00007fff8d15a3de AppKit`_DPSNextEvent + 1434
    frame #30: 0x00007fff8d159a2b AppKit`-[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] + 122
    frame #31: 0x0000000102b49b57 XUL`-[GeckoNSApplication nextEventMatchingMask:untilDate:inMode:dequeue:](self=0x0000000100125460, _cmd=0x00007fff8db8d5c3, mask=18446744073709551615, expiration=0x422d63c37f00000d, mode=0x00007fff76e59d00, flag='\x01') + 119 at nsAppShell.mm:165
    frame #32: 0x00007fff8d14db2c AppKit`-[NSApplication run] + 553
    frame #33: 0x0000000102b4b6d2 XUL`nsAppShell::Run(this=0x0000000100159c30) + 162 at nsAppShell.mm:742
    frame #34: 0x000000010480f07c XUL`nsAppStartup::Run(this=0x0000000100158ef0) + 156 at nsAppStartup.cpp:276
    frame #35: 0x00000001046c7bdc XUL`XREMain::XRE_mainRun(this=0x00007fff5fbff050) + 6044 at nsAppRunner.cpp:4008
    frame #36: 0x00000001046c83e9 XUL`XREMain::XRE_main(this=0x00007fff5fbff050, argc=2, argv=0x00007fff5fbff958, aAppData=0x00007fff5fbff2e8) + 697 at nsAppRunner.cpp:4075
    frame #37: 0x00000001046c885d XUL`XRE_main(argc=2, argv=0x00007fff5fbff958, aAppData=0x00007fff5fbff2e8, aFlags=0) + 77 at nsAppRunner.cpp:4285
    frame #38: 0x0000000100002117 firefox`do_main(argc=2, argv=0x00007fff5fbff958, xreDirectory=0x000000010011e220) + 1639 at nsBrowserApp.cpp:282
    frame #39: 0x0000000100001651 firefox`main(argc=2, argv=0x00007fff5fbff958) + 321 at nsBrowserApp.cpp:643
    frame #40: 0x00000001000010b4 firefox`start + 52
Ah, so the overflow updates triggered by sticky positioning end up traversing too far, in particular to that nsHTMLScrollFrame @ 0x000000011b329c70. It would be useful to cross-reference the stack trace with a frame tree dump to see whether that's the root scroll frame or the "overflow-x:hidden;position:sticky;" element itself.

Possibly we're missing a GetParent() call somewhere such that some part of the code thinks the sticky element is its own scroll container?
The StickyScrollContainer assigned to the root scroll frame was created with StickyScrollContainer::mFrames containing the innermost nsHTMLScrollFrame (which has "overflow-x:hidden;position:sticky;".
nsHTMLScrollFrame::UpdateOverflow() detects that its scroll frame will need to be re-flowed when the overflow area changes and there are scroll bars or the scroll offset is not at the origin.  The assert occurs due to calling nsMathMLContainerFrame::UpdateOverflow() during a reflow.

The example testcase can be corrected by adding an overflow-y:hidden to bypass this branch.  This results in the image appearing and the assert no longer occurring.

A similar problem may occur with nsMathMLContainerFrame::UpdateOverflow(), which also calls FrameNeedsReflow()
Perhaps this issue can be resolved by deferring the call to FrameNeedsReflow with PresShell::PostReflowCallback; however, perhaps a cleaner solution could solve it without additional passes.
Why are we using UpdateOverflow on frames currently being reflowed?  It should only be used outside of reflow or on  frames not currently being reflowed; if a frame is currently being reflowed the reflow will take care of updating overflow.
So the way this is supposed to work, starting from nsHTMLScrollFrame::Reflow, is:
 1. At the end of reflowing a scroll frame (after reflowing its contents), we call StickyScrollContainer::UpdatePositions.
 2. UpdatePositions sets up an OverflowChangedTracker with the scroll frame as mSubtreeRoot. As each sticky frame is positioned, it's added to the OverflowChangedTracker.
 3. After that's all done, OverflowChangedTracker::Flush updates overflow areas, walking up the frame tree to (but not including) mSubtreeRoot.
 4. Then we can return all the way back to nsHTMLScrollFrame::Reflow, update the scroll frame's overflow areas, and continue on.

We indeed ought not to be calling UpdateOverflow on frames currently being reflowed. In the test case, maybe something about the <img> or <table> breaks some of my assumptions about how reflow works?
Huh, looks like merely "overflow-x:hidden;position:sticky;" is sufficient to trigger that assertion (and then it's unsurprising that that causes problems for later reflows, like the one when the image is loaded).
And looking at Kip's stack trace from comment 6 again, we have
> nsHTMLScrollFrame::UpdateOverflow(this=0x000000011b329c70)
> nsHTMLScrollFrame::Reflow(this=0x0000000112d3ad88, ...)

I'd guess that the scroll frame Reflow was called on is the root scroll frame (since per comment 8 this does indeed have a StickyScrollContainer attached). But that would mean we're hitting the assertion when calling UpdateOverflow on the sticky frame. Why would we still be reflowing it at that point?
The assert is testing a variable, nsPresShell::mIsReflowing, which does not track individual frames that are reflowing.

Could it be that a different instance of OverflowChangedTracker should be used, which is flushed outside of the reflow window?  Perhaps RestyleManager::mOverflowChangedTracker or one instantiated by nsPresShell?
Following some IRC discussion, it seems the part I hadn't paid enough attention to was that ScrollFrameHelper::UpdateOverflow() can call PresShell::FrameNeedsReflow() when it thinks the scroll position could have changed:
https://hg.mozilla.org/mozilla-central/file/b01286b4ed37/layout/generic/nsGfxScrollFrame.cpp#l3994

So, given a sticky frame that also has scrollbars in at least one dimension, when we position it as part of reflowing its scroll container, the overflow-updating hits this logic and leads to the assertion.

My initial thought is that that logic in ScrollFrameHelper::UpdateOverflow() seems unnecessarily cautious in this situation. We're updating the overflow areas of the scrollable+sticky frame only because we changed its position, not its contents or its size, and so it doesn't seem that its scroll position could have changed. I wonder if we can detect that that's all that's happened and that it's safe to skip the reflow?
Or... since we're only changing the position of sticky frames, maybe we could get away with not calling UpdateOverflow on them at all, but instead starting with their parents? (Looks like that might involve tweaking the semantics of OverflowChangedTracker, though.)
I have experimented with adding a parameter to nsIFrame::UpdateOverflow to allow the caller to hint if only the position of the overflow is expected to be updated.  The experiment included a similar parameter to pass the hint through OverflowChangedTracker::Flush when called from StickyScrollContainer::UpdatePositions.  It appears to correct the disappearing image in the testcase and eliminates the assertion.

I am digging a bit further to see if there are further side-effects and that the sticky scroll frames will be reflowed when necessary with the patch in place.

Would this warrant a change to an interface (nsIFrame)?  Perhaps Corey's solution, localized to OverflowChangedTracker, would be less intrusive and preferable?
This proposed fix is experimental.  Additional testcases and reftests will be added to confirm if it is effective and without side effects.

Also please see comment 17 and 18 for alternate solution which may be less invasive.
Attachment #8390774 - Flags: review?(corey)
Comment on attachment 8390774 [details] [diff] [review]
V1 fix for Bug 926155 (experimental)

I believe overflow areas are stored relative to the frame's position, so when we position sticky frames we should actually expect their overflow areas not to change at all. So I don't think this is the right approach.
Attachment #8390774 - Flags: review?(corey)
The fact that MathML can (indeed, always does) fall back to a full reflow means that the just-start-with-the-parent(s) idea won't suffice either -- sticky inside MathML triggers the same assertion,
Or rather, starting overflow updates from sticky frames' parents seems like a sensible solution to the (original) scrollable+sticky scenario.

I don't know why nsMathMLContainerFrame::UpdateOverflow needs to fall back to a reflow, but some options there might include (a) optimizing it to not reflow (b) deferring that reflow to another pass (c) disallowing position:sticky on/in MathML.

(Ultimately, this all appears to come from the fact that UpdateOverflow was designed as an optimization to avoid full reflows where possible, yet the position:sticky implementation assumed that UpdateOverflow could be safely used while another reflow was in progress.)

dbaron, assuming the problem makes sense to you now, do you have any ideas?
Flags: needinfo?(dbaron)
Patch submitted for Bug 984226 partially corrects this issue: https://bugzilla.mozilla.org/attachment.cgi?id=8393971

In addition to that patch need reftest / mochitest and need to address issue demonstrated in the MathML testcase.
Attachment #8394027 - Flags: review?(corey)
Reftest has been updated to use MozReftestInvalidate instead of setTimeout
Attachment #8394027 - Attachment is obsolete: true
Attachment #8394027 - Flags: review?(corey)
Attachment #8394046 - Flags: review?(dholbert)
Comment on attachment 8394046 [details] [diff] [review]
V2 Test for Bug 926155 - Non MathML test case

Two extreme nits:

>+++ b/layout/reftests/bugs/926155-1.html
>+    function doTest() {
>+      var x=document.getElementById('testdiv');
>+      x.style.width="200px";
>+      document.documentElement.className = "";

Mozilla coding style is to have spaces around operators like "=", so I'd suggest adding spaces around the "=" in the assignment for x and x.style.width. (for consistency with the last line, too)

>+++ b/layout/reftests/bugs/reftest.list
> == 921716-1.html 921716-1-ref.html
> fuzzy-if(cocoaWidget,1,40) == 928607-1.html 928607-1-ref.html
>+== 926155-1.html 926155-1-ref.html
> == 931464-1.html 931464-1-ref.html

This is inserting 1 line too low, for sorted order.

(926155-1.html should go before 928607-1.html)

r=me with those fixed. Thanks!
Attachment #8394046 - Flags: review?(dholbert) → review+
With bug 63895 landed (including a new use of OverflowChangedTracker), I wonder whether we can similarly trigger this assertion with absolutely-positioned elements inside scrollframes/MathML inside relatively-positioned table parts, or something like that.
Updated with feedback in Comment 28
Attachment #8394046 - Attachment is obsolete: true
Depends on: 984226
Assignee: nobody → kgilbert
Comment on attachment 8390774 [details] [diff] [review]
V1 fix for Bug 926155 (experimental)

Patch obsoleted by fix applied in Bug 984226
Attachment #8390774 - Attachment is obsolete: true
Comment on attachment 8390925 [details] [diff] [review]
Alternative fix for Bug 926155, based on Comment 17

Patch obsoleted by fix applied in Bug 984226
Attachment #8390925 - Attachment is obsolete: true
Comment on attachment 8391093 [details]
asserting testcase with sticky inside MathML

A separate bug has been entered related to this testcase, Bug 997893.  The testcase attachment has also been moved there.
Attachment #8391093 - Attachment is obsolete: true
This bug has been corrected with Bug 984226.

The attached reftest, bug926155_reftest.patch, needs check-in.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d32e6e5c5617
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: