I have a very large pile of patches that do this. They pass tests on the tryservers and seem to work well. The basic idea is that all the scrolling API moves out of nsIScrollableView into nsIScrollableFrame. All the functionality of nsScrollPortView moves into nsGfxScrollFrame.

I don't think this needs review.

This adds every API that we need from nsIScrollableView to nsIScrollableFrame, and implements it in the existing infrastructure. This needs careful review, I hope you like the API!

This obsoletes nsIScrollableViewProvider (which I'll remove later in the patch series).

Switch to new APIs in PresShell, nsLayoutUtils, and nsSelection (but not completely, there's more to do in later patches)

-- Use nsIScrollableFrame::ScrollUnit instead of nsEventStateManager::ScrollQuantity -- Remove #if NON_KEYBINDING code -- Update to new APIs

I need this to fix some bogus "am I the root element or BODY" code in nsNSElementTearoff in the next patch.

Also fixes things so that we check specifically for "the root element" and "the body element" instead of doing something weird for any element that happens to have tag name "html" or "body".

Asking Mats for review since this is all about layout code really.

Again, requesting review from Mats because this is basically simple layout-related changes.

A lot more to come in nsSelection later in the patch series --- it's a real view-fest.

Now we can finally do something fun. We've removed all users of nsIScrollableViewProvider, so now we can kill it.

No-one calls nsIViewManager::GetRootScrollableView anymore.

Make nsRect::IntersectRect preserve height and width if possible even when the intersection of two rectangles is empty. We need this for some later patches that scroll selection into view and need to do meaningful things with zero-width rects. Asking Mats for review since nsRect is really layout...

GetCaretCoordinates is grotesque and tied to views.

We need this to reimplement scrolling of selections into view, without using scrollviews.

At this point no-one outside of nsGfxScrollFrame uses nsIScrollableView anymore, so we can remove access to it.

Comment on attachment 410132 [details] [diff] [review] Part 6: Convert nsEventStateManager to new APIs > static PRBool >-CanScrollOn(nsIScrollableView* aScrollView, PRInt32 aNumLines, >+CanScrollInRange(nscoord aMin, nscoord aValue, nscoord aMax, PRInt32 aDirection) >+{ >+ return aDirection > 0 ? aValue < aMax : aMin < aValue; >+} >+ >+static PRBool >+CanScrollOn(nsIScrollableFrame* aScrollFrame, PRInt32 aNumLines, > PRBool aScrollHorizontal) > { >- NS_PRECONDITION(aScrollView, "aScrollView is null"); >+ NS_PRECONDITION(aScrollFrame, "aScrollFrame is null"); > NS_PRECONDITION(aNumLines, "aNumLines must be non-zero"); >- PRBool canScroll; >- nsresult rv = >- aScrollView->CanScroll(aScrollHorizontal, aNumLines > 0, canScroll); >- return NS_SUCCEEDED(rv) && canScroll; >+ nsPoint scrollPt = aScrollFrame->GetScrollPosition(); >+ nsRect scrollRange = aScrollFrame->GetScrollRange(); >+ >+ return aScrollHorizontal >+ ? CanScrollInRange(scrollRange.x, scrollPt.x, scrollRange.XMost(), aNumLines) >+ : CanScrollInRange(scrollRange.y, scrollPt.y, scrollRange.YMost(), aNumLines); > } Would it make sense to move this to nsIScrollableFrame. I could have method PRBool CanScroll(PRBool aHorizontal, PRBool aForward) So, basically the same what nsIScrollableView has now. >- if (!passToParent && scrollView) { >- if (aScrollQuantity == eScrollByLine) { >+ if (!passToParent && frameToScroll) { >+ if (aScrollQuantity == nsIScrollableFrame::LINES) { > numLines = > nsMouseWheelTransaction::AccelerateWheelDelta(numLines, isHorizontal, > aAllowScrollSpeedOverride, > &aScrollQuantity); >+ // Limit scrolling to be at most one page, but if possible, try to >+ // just adjust the number of scrolled lines. >+ nscoord lineHeight = frameToScroll->GetLineScrollAmount().height; >+ if (lineHeight) { >+ nsSize pageScrollAmount = frameToScroll->GetPageScrollAmount(); >+ nscoord pageScroll = isHorizontal ? >+ pageScrollAmount.width : pageScrollAmount.height; >+ >+ if (PR_ABS(numLines) * lineHeight > pageScroll) { >+ nscoord maxLines = (pageScroll / lineHeight); >+ if (maxLines >= 1) { >+ numLines = ((numLines < 0) ? -1 : 1) * maxLines; >+ } else { >+ aScrollQuantity = nsIScrollableFrame::PAGES; >+ } >+ } >+ } I don't understand this change. AccelerateWheelDelta calls LimitToOnePageScroll. Why do we still need to limit scrolling to one page here?

Creates nsLayoutUtils::IsPopup and nsLayoutUtils::GetDisplayRootFrame.

The height and width components of the rect are never used.

This is the big patch. Basically, most of the code from nsScrollPortView is migrated to nsGfxScrollFrameInner. Since we don't have an anonymous inner scrollport view anymore, the scrollport bounds are stored in mScrollPort. We reimplement the nsIScrollableFrame methods directly. The merge simplifies many things; for example, nsGfxScrollFrameInner no longer needs to be an nsIScrollPositionListener.

We no longer need nsIView::SetInvalidateFrameOnScroll etc, scrollframes are checking for transformed ancestors directly. nsCSSFrameConstructor::FinishBuildingScrollFrame no longer needs to construct a view for the scrolled frame. nsIFrame::GetParentViewForChildFrame is no longer needed. View construction is simplified because there is no longer any need to call SetScrolledView.

(In reply to comment #29) > Would it make sense to move this to nsIScrollableFrame. > I could have method > PRBool CanScroll(PRBool aHorizontal, PRBool aForward) > So, basically the same what nsIScrollableView has now. We only need it here. I'd rather keep the interface minimal. > I don't understand this change. AccelerateWheelDelta calls > LimitToOnePageScroll. Why do > we still need to limit scrolling to one page here? That's basically just a mismerge. I'll delete this code.

Comment on attachment 410200 [details] [diff] [review] Part 6 v2 In my mind the CanScroll clearly belongs to nsIScrollableFrame. But ok.

Two issues I found so far: 1) nsContainerFrame::FrameNeedsView forces views for scrolledframes. We should nix that. 2) A crash on restoring from bfcache due to this code in nsHTMLScrollFrame::PostScrolledAreaEventForCurrentArea 880 nsRect currentScrolledArea = mInner.mScrolledFrame->GetView()->GetBounds(); once issue #1 is fixed (because there is no view).

We can use a simpler event object. We don't need to store the scrolled area, since we can just compute it when we fire the event. Computing it without using the view avoids the crash Boris mentioned. (That code was added after I first converted nsGfxScrollFrame.)

Update part 32 so we don't create views for scrolled frames, as Boris mentioned.

Frames with transforms don't need views either anymore?

Correct, as far as I know. They used to have views so we could set the "scrolling descendants can't bitblit" bit, but that's not needed anymore --- nsGfxScrollFrame just checks the ancestor frames explicitly.

Will have some review comments soon; meanwhile you might want to have a look at these (local Linux x86-64 debug build, in case it matters): Mochitest content/html/document/test/test_bug512367.html triggers: ###!!! ASSERTION: curPos.x not a multiple of device pixels: 'curPosDevPx.x*appUnitsPerDevPixel == curPos.x', nsGfxScrollFrame.cpp, line 1717 ###!!! ASSERTION: curPos.y not a multiple of device pixels: 'curPosDevPx.y*appUnitsPerDevPixel == curPos.y', nsGfxScrollFrame.cpp, line 1719 Mochitest content/base/test/test_bug340571.html (and many others) triggers: ###!!! ASSERTION: Scrolled rect smaller than scrollport?: 'result.height >= mScrollPort.height', nsGfxScrollFrame.cpp, line 2989

(In reply to comment #43) > Mochitest content/html/document/test/test_bug512367.html triggers: > ###!!! ASSERTION: curPos.x not a multiple of device pixels: > 'curPosDevPx.x*appUnitsPerDevPixel == curPos.x', nsGfxScrollFrame.cpp, line > 1717 > ###!!! ASSERTION: curPos.y not a multiple of device pixels: > 'curPosDevPx.y*appUnitsPerDevPixel == curPos.y', nsGfxScrollFrame.cpp, line > 1719 Normally we maintain the invariant that the scroll position is a multiple of device pixels. Here that invariant's breaking because we've scrolled a bit, then we change the zoom factor so the current scroll position is no longer a multiple of device pixels. What we probably need to do is to fix the scroll position during the reflow triggered by zooming.

Fix those two assertions: -- Make nsHTMLScrollFrame::Reflow snap the current scroll position to device pixels in case the reflow is due to a change in zoom factor -- GetScrolledRect gets called in some situations where the overflow area of the scrolled frame is not set correctly. In those situations, call GetScrolledRectInternal passing in what the overflow area is going to be.

Comment on attachment 410127 [details] [diff] [review] Part 3: Add all necessary APIs to nsIScrollableFrame >-/* >- * interface for rendering objects that wrap rendering objects that should >- * be scrollable >- */ Those are useful; they show up in LXR. Could you leave it, or fix it appropriately? See http://mxr.mozilla.org/mozilla-central/source/layout/generic/ >+ * @param aOverflow if non-null, returns the amount that scrolling >+ * was clamped by in each direction. This is never negative. This >+ * is only supported for LINES and DEVICE_PIXELS. The values are in >+ * device pixels. Could you define "clamped" better? And what units is aOverflow in? aUnit or always device pixels? (And why doesn't it work with PAGES?) >+ * XXX should we atke an aMode parameter here? Currently it's instant. take I don't quite get the pattern of when you're putting a blank line before new doc comments in nsIScrollableFrame.h and when you're not, though perhaps there's a reason. If not, maybe a blank line before each? sr=dbaron

Comment on attachment 410128 [details] [diff] [review] Part 4: add nsIFrame::GetScrollTargetFrame >+// XXXroc this is megalame. Fossicking around for a frame of the right Choosing words that are in the dictionary might make things a little more approachable to non-native speakers. :-) rev nsIFrame's IID, and sr=dbaron

(In reply to comment #47) > (From update of attachment 410128 [details] [diff] [review]) > >+// XXXroc this is megalame. Fossicking around for a frame of the right > > Choosing words that are in the dictionary might make things a little more > approachable to non-native speakers. :-) But I guess that comment was copied from existing code, too. I'm assuming that existing code gets removed in one of the other 30 patches?

(In reply to comment #48) > I'm assuming that existing code gets removed in one of the other 30 patches? Yes, part 18. (In reply to comment #47) > rev nsIFrame's IID Will do. NS_IFRAME_IID is actually unused now, I guess since the switch to nsQueryFrame. So I'll just remove it. (In reply to comment #46) > (From update of attachment 410127 [details] [diff] [review]) > >-/* > >- * interface for rendering objects that wrap rendering objects that should > >- * be scrollable > >- */ > > Those are useful; they show up in LXR. Could you leave it, or fix it > appropriately? Fascinating. Will fix. > >+ * @param aOverflow if non-null, returns the amount that scrolling > >+ * was clamped by in each direction. This is never negative. This > >+ * is only supported for LINES and DEVICE_PIXELS. The values are in > >+ * device pixels. > > Could you define "clamped" better? Yes. > And what units is aOverflow in? Device pixels, as it says. > (And why doesn't it work with PAGES?) Because nsScrollPortView::ScrollByPages doesn't support it. Part 31 removes this limitation. > I don't quite get the pattern of when you're putting a blank line before > new doc comments in nsIScrollableFrame.h and when you're not, though > perhaps there's a reason. I was trying to group together related members.

This testcase doesn't work in my tree with the patch set applied. (my review is almost complete, comments in bit...)

Comment on attachment 410151 [details] [diff] [review] Part 23: fix nsRect::IntersectRect part 23: This patch looks a bit scary. David, can you have a look too please? This comment about width/height is incorrect: http://mxr.mozilla.org/mozilla-central/source/layout/xul/base/src/nsMenuPopupFrame.cpp#1067 is the code still correct? Maybe early return here if dirty.IsEmpty(): http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSRendering.cpp#2020

part 1: ok part 2: ok part 3: > nsIScrollableFrame.h: + * XXX should we atke an aMode parameter here? Currently it's instant. typo part 4: > nsComboboxControlFrame.h: + virtual nsIScrollableFrame* GetScrollTargetFrame() { + nsIScrollableFrame* scrollable = do_QueryFrame(mDropdownFrame); + return scrollable; + } "return do_QueryFrame(mDropdownFrame);" is shorter. part 5: > nsLayoutUtils.cpp: + // If scrolling in a specific direction, require visible scrollbars or + // something to scroll to in that direction. Suggest "Require visible scrollbars or something to scroll to in the given direction." now that "eEither" is handled separately. > nsLayoutUtils.h: + * GetNearestScrollableFrame locates the first ancestor of aFrame + * (or aFrame itself) that is scrollable and overflow:scroll or s/and/with/ > nsPresContext.h: + PRBool operator==(const ScrollbarStyles& aStyles) { + PRBool operator!=(const ScrollbarStyles& aStyles) { Can be const methods? > nsPresShell.cpp // Utility to find which view to scroll. - nsIScrollableView* GetViewToScroll(nsLayoutUtils::Direction aDirection); + nsIScrollableFrame* GetFrameToScroll(nsLayoutUtils::Direction aDirection); Fix/remove the comment. +PresShell::GetFrameToScroll There's a subtle change here compared to the old GetViewToScroll. In the "focusedContent case", the old code looks for a scrollable view on the primary frame and calls GetNearestScrollingView(... aDirection), which only searches in aDirection. The new code calls GetScrollTargetFrame() (which is not limited to aDirection) and returns that if non-null. Is this change intentional? + nscoord scrollViewLineHeight = s/scrollViewLineHeight/scrollFrameLineHeight/ part 10: ok part 11: ok part 12: > nsTreeBodyFrame.cpp nsGfxScrollFrameInner::ScrollToImpl does PostScrollEvent(), so I don't think we need that in nsTreeBodyFrame::ScrollHorzInternal. part 13: ok part 14: ok part 15: ok part 16: ok part 17: > nsTextControlFrame.cpp In nsTextInputSelectionImpl::nsTextInputSelectionImpl(): + mScrollFrame = nsnull; Please change this to an (unconditional) member init. part 18: ok part 19: ok part 20: ok Part 21: > nsAccessible.cpp + if (isEmpty && frame && !(frame->GetStateBits() & NS_FRAME_OUT_OF_FLOW)) { No need to null-check 'frame' there. +PresShell::GetRectVisibility Why change '<' to '<=' for the "kAboveViewport" test (etc)? Part 22: > nsAccessible.cpp - PRBool isVisible = CheckVisibilityInParentChain(doc, frame->GetClosestView()); + nsIView* view = frame->GetAncestorWithViewExternal()->GetViewExternal(); GetClosestView() checks 'this', GetAncestorWithViewExternal() does not. Is 'frame' guarranteed to not be the root frame here? > nsPresShell.cpp In AccumulateFrameBounds(): // If this is an inline frame and either the bounds height is 0 (quirks // layout model) or aVPercent is not NS_PRESSHELL_SCROLL_ANYWHERE, we need to // change the top of the bounds to include the whole line. - if (frameBounds.height == 0 || aVPercent != NS_PRESSHELL_SCROLL_ANYWHERE) { + if (frameBounds.height == 0 || aUseWholeLineHeightForInlines) { The comment refers to the removed 'aVPercent'. part 23: see separate comment above. part 24: ok part 25: ok part 26: > nsSelection.cpp In nsAutoScrollTimer::Notify() - if (!frame.IsAlive()) - return NS_OK; We still need this check - 'frame' is used on the next line. (with that we can have NS_PRECONDITION(aFrame, "Need a frame") in DoAutoScroll too) part 27: ok part 28: ok part 29: > nsLayoutUtils.h + * A frame is a popup if it has its own floating window. Menus, panels + * and combobox dropdowns are popups. The comment mentions panels, the code doesn't seem to do that. part 30: ok part 31 v2: > nsGfxScrollFrame.cpp In nsGfxScrollFrameInner::ScrollBy(): + ScrollTo(newPos, aMode); + if (aOverflow) { - *aOverflow = overflow; + nsPoint clampAmount = mDestination - newPos; ScrollTo may have destroyed 'this' if 'aMode' is INSTANT. We should update aOverflow also for WHOLE, or say in the header that it isn't supported for WHOLE (and in that case assert that it's null). In nsGfxScrollFrameInner::GetPageScrollAmount(): + mScrollPort.width - PR_MIN(mScrollPort.width/10, ... + mScrollPort.height - PR_MIN(mScrollPort.height/10, ... NS_MIN is better. In nsGfxScrollFrameInner::ScrollToRestoredPosition()(): + ScrollTo(mRestorePos, nsIScrollableFrame::INSTANT); + // Re-get the scroll position, it might not be exactly equal to + // mRestorePos due to rounding and clamping. + mLastPos = GetScrollPosition(); ScrollTo may have destroyed 'this'. > nsGfxScrollFrame.h + // Update scrollbar curpos attributes to reflect current scroll + // position This comment fits on one line. part 32: ok part 33: ok No specific part: nsIPresShell.h needs to bump NS_IPRESSHELL_IID nsFrameSelection.h needs to bump NS_FRAME_SELECTION_IID nsIScrollPositionListener.h needs to bump NS_ISCROLLPOSITIONLISTENER_IID Maybe nsView.cpp needs to bump VIEW_WRAPPER_IID since we removed nsIScrollableView in ViewWrapper::GetInterface()? Maybe nsDocAccessible.h needs to bump NS_DOCACCESSIBLE_IMPL_CID? Finally, I want to take another pass looking at frame liveness specifically. Should be done by tomorrow with that.

Comment on attachment 410151 [details] [diff] [review] Part 23: fix nsRect::IntersectRect sr=dbaron

(In reply to comment #51) > This comment about width/height is incorrect: > http://mxr.mozilla.org/mozilla-central/source/layout/xul/base/src/nsMenuPopupFrame.cpp#1067 > is the code still correct? Perhaps not. I'll fix it to set the width and height to zero there. > Maybe early return here if dirty.IsEmpty(): > http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSRendering.cpp#2020 Sure. (In reply to comment #52) > typo Fixed > "return do_QueryFrame(mDropdownFrame);" is shorter. OK > Suggest "Require visible scrollbars or something to scroll to in > the given direction." now that "eEither" is handled separately. Good, fixed. > s/and/with/ Fixed > Can be const methods? Yes, fixed > Fix/remove the comment. Removed > +PresShell::GetFrameToScroll > > There's a subtle change here compared to the old GetViewToScroll. > In the "focusedContent case", the old code looks for a scrollable view > on the primary frame and calls GetNearestScrollingView(... aDirection), > which only searches in aDirection. The new code calls > GetScrollTargetFrame() (which is not limited to aDirection) and returns > that if non-null. Is this change intentional? No. That's a very good catch. We should start our search at the result of GetScrollTargetFrame (if it's non-null) but always call GetNearestScrollableFrameForDirection whether GetScrollTargetFrame returned null or not. > s/scrollViewLineHeight/scrollFrameLineHeight/ Fixed > part 12: > > nsTreeBodyFrame.cpp > nsGfxScrollFrameInner::ScrollToImpl does PostScrollEvent(), so I don't > think we need that in nsTreeBodyFrame::ScrollHorzInternal. You mean nsGfxScrollFrameInner::ScrollToImpl in aParts.mColumnsScrollFrame? The event doesn't bubble so I don't think that will reach the tree. Anyway, I wouldn't want to rely on that. > Please change this to an (unconditional) member init. Fixed > Part 21: > > +PresShell::GetRectVisibility > > Why change '<' to '<=' for the "kAboveViewport" test (etc)? If aMinTwips is 0 and r.YMost() == insetRect.y (which == scrollPortRect.y), then the frame is not visible. Right? > Part 22: > > nsAccessible.cpp > - PRBool isVisible = CheckVisibilityInParentChain(doc, > frame->GetClosestView()); > + nsIView* view = frame->GetAncestorWithViewExternal()->GetViewExternal(); > > GetClosestView() checks 'this', GetAncestorWithViewExternal() does not. > Is 'frame' guarranteed to not be the root frame here? Good catch, I'll just restore the old behavior of return frame's view if frame->HasView(). > The comment refers to the removed 'aVPercent'. Fixed. > We still need this check - 'frame' is used on the next line. > (with that we can have NS_PRECONDITION(aFrame, "Need a frame") > in DoAutoScroll too) Great catch, fixed. > part 29: > > nsLayoutUtils.h > > + * A frame is a popup if it has its own floating window. Menus, panels > + * and combobox dropdowns are popups. > > The comment mentions panels, the code doesn't seem to do that. Panels are just nsMenuPopupFrames. > part 31 v2: > > nsGfxScrollFrame.cpp > In nsGfxScrollFrameInner::ScrollBy(): > + ScrollTo(newPos, aMode); > + > if (aOverflow) { > - *aOverflow = overflow; > + nsPoint clampAmount = mDestination - newPos; > > ScrollTo may have destroyed 'this' if 'aMode' is INSTANT. How would ScrollTo destroy 'this'? > We should update aOverflow also for WHOLE, or say in the header that > it isn't supported for WHOLE (and in that case assert that it's null). I'll just return 0,0 in *aOverflow for WHOLE. > NS_MIN is better. Fixed > In nsGfxScrollFrameInner::ScrollToRestoredPosition()(): > + ScrollTo(mRestorePos, nsIScrollableFrame::INSTANT); > + // Re-get the scroll position, it might not be exactly equal to > + // mRestorePos due to rounding and clamping. > + mLastPos = GetScrollPosition(); > > ScrollTo may have destroyed 'this'. As above, I don't think ScrollTo can destroy the frame. > This comment fits on one line. Fixed > nsIPresShell.h needs to bump NS_IPRESSHELL_IID Part 21 bumps it. But Part 25 should too, so I've done it there as well. > nsFrameSelection.h needs to bump NS_FRAME_SELECTION_IID Does it? We don't change any virtual methods. > nsIScrollPositionListener.h needs to bump NS_ISCROLLPOSITIONLISTENER_IID Done. > Maybe nsView.cpp needs to bump VIEW_WRAPPER_IID since we removed > nsIScrollableView in ViewWrapper::GetInterface()? Sure, why not :-). > Maybe nsDocAccessible.h needs to bump NS_DOCACCESSIBLE_IMPL_CID? I don't think so. Which parts do you want me to repost?

The testcase in comment #50 fails because scroll positions are now forced to be multiples of device pixels, but GetScrollRange returns values which aren't rounded to device pixels. So when an element whose height is not an integer number of device pixels is scrolled as far down as possible, the scroll position y is still less than GetScrollRange().YMost() so CanScrollOn returns true.

> >> > > nsTreeBodyFrame.cpp > > > nsGfxScrollFrameInner::ScrollToImpl does PostScrollEvent(), so I don't > > > think we need that in nsTreeBodyFrame::ScrollHorzInternal. > You mean nsGfxScrollFrameInner::ScrollToImpl in aParts.mColumnsScrollFrame? Yes. > The event doesn't bubble so I don't think that will reach the tree. Ok, but I thought 'aParts.mColumnsScrollFrame' and 'this' in nsTreeBodyFrame::ScrollHorzInternal() had the same content? If not, then disregard my comment. > How would ScrollTo destroy 'this'? We already had this discussion in bug 435422 ;-) I still believe ScrollToImpl can destroy the world. (related: bug 421839, bug 402505, bug 269832) I think the call to ScrollTo() in ScrollToRestoredPosition() and ScrollBy() should have a nsWeakFrame check around it. > > Why change '<' to '<=' for the "kAboveViewport" test (etc)? > If aMinTwips is 0 and r.YMost() == insetRect.y (which == scrollPortRect.y), > then the frame is not visible. Right? Ah, right, so the old code has a bug? > > nsFrameSelection.h needs to bump NS_FRAME_SELECTION_IID > Does it? We don't change any virtual methods. No, you're right. > Which parts do you want me to repost? Please attach "hg diff -r qparent" instead and I'll diff that to the current one.

(In reply to comment #56) > > The event doesn't bubble so I don't think that will reach the tree. > > Ok, but I thought 'aParts.mColumnsScrollFrame' and 'this' in > nsTreeBodyFrame::ScrollHorzInternal() had the same content? > If not, then disregard my comment. Hmm ... I'm not sure without checking, and anyway, we shouldn't rely on such a subtle thing. > > How would ScrollTo destroy 'this'? > > We already had this discussion in bug 435422 ;-) > I still believe ScrollToImpl can destroy the world. > (related: bug 421839, bug 402505, bug 269832) The summary of bug 435422 is that it happens via "mWidget->Invalidate(PR_TRUE);" in nsPluginInstanceOwner::ScrollPositionDidChange, which is only on Mac right? I think we should just remove that Invalidate call completely. I don't see why it's necessary. Bug 421839 doesn't show ScrollTo/ScrollToImpl destroying anything. Nor does bug 402505 or bug 269832, as far as I can tell. I suppose it is possible that on Mac, doing a SetWindow call to the plugin in Will/DidChangeScrollPosition would cause the plugin to do something nasty like trigger JS. But we can't really protect ourselves from that. > I think the call to ScrollTo() in ScrollToRestoredPosition() and > ScrollBy() should have a nsWeakFrame check around it. The problem is that then all the callers to ScrollBy and ScrollToRestoredPosition need to be audited to use weak frames as well. It gets really messy. > > > Why change '<' to '<=' for the "kAboveViewport" test (etc)? > > If aMinTwips is 0 and r.YMost() == insetRect.y (which == scrollPortRect.y), > > then the frame is not visible. Right? > > Ah, right, so the old code has a bug? I think so. > > Which parts do you want me to repost? > > Please attach "hg diff -r qparent" instead and I'll diff that to > the current one. I'll do that once I've integrated the fix for your failing testcase.

(In reply to comment #57) > > > How would ScrollTo destroy 'this'? The stack in bug 490461 shows that there's a path from ScrollToImpl() to nsFocusManager::SetFocusInner() which calls CheckIfFocusable() which does "doc->FlushPendingNotifications(Flush_Frames)" which can do nasty things, right?

In that stack, Windows is dispatching WM_MOUSEACTIVATE while we call ::UpdateWindow(). That is evil (and undocumented) :-(. Maybe something to do with the Acrobat plugin running out-of-process? Maybe the solution is to rip out the Composite() call and not bother. I really don't want to carefully make lots of code resistant to flushes during scrolling and then go and make the visual part of scrolling asynchronous so we don't need it anymore.

I think we should try disabling Composite() and see what happens. If the results aren't too bad, that will do.

As requested

Comment on attachment 416251 [details] [diff] [review] Part 0: Remove Composite() call r=mats

Your updates in attachment 416252 [details] [diff] [review] looks good, r=mats on the remaining parts where my review was requested.

I checked in part 0: http://hg.mozilla.org/mozilla-central/rev/56a6449e4833

I'm not really comfortable with part 0. But I don't see any other way to avoid the reentrancy issues if UpdateWindow can post arbitrary non-Paint messages to the window, as bug 490461 indicates.

Part 0 caused regression bug 536065. It seems we're going to have to back it out and just audit a bazillion code paths :-(.

One problematic path is where nsScrollbarFrame::AttributeChanged calls nsGfxScrollFrameInner::CurPosAttributeChanged which calls nsGfxScrollFrameInner::ScrollTo(INSTANT), which can destroy the world. We can't make AttributeChanged safe for arbitrary script execution, obviously. I suppose we can just assume that the scrollbar mode is always "smooth" so that scrolling happens asynchronously, but that might feel laggy. But I don't see we have much choice.

Could you use script runner and not fully asynchronous thread dispatching?

That's a good idea, I'll look into it. Thanks!

Making nsIScrollableFrame::ScrollTo/ScrollBy possibly destroy the world is incredibly painful. The amount of code that would have to be modified to handle potential frame destruction is really large and testing it all to make sure it handles frame destruction would be really, really hard. We've got to just not do this. Here is Mats' problematic stack: https://bug490461.bugzilla.mozilla.org/attachment.cgi?id=416005 I think it's significant that the only stack we have of this _NtUserCallHwndLock involves a plugin (Adobe Acrobat Reader) which uses HWNDs bound to a non-main thread. Also, the event being dispatched during _NtUserCallHwndLock is WM_MOUSEACTIVATE, which has the behaviour that DefWindowProc dispatches it to the parent window before processing in the child. So I suspect that what happens there is that WM_MOUSEACTIVATE is sent to a non-main-thread window of Acrobat, which sends WM_MOUSEACTIVATE to our plugin window, and our call to UpdateWindow decides to process that event. So I think it should be enough to add a script blocker around the call to UpdateViewAfterScroll and have our PluginWndProc WM_MOUSEACTIVE handler ignore the event if it's not safe to run scripts.

I want to be able to add a script blocker to signal that it's not safe to run scripts, in a situation where code might test whether it's safe to run scripts but will not (or at least, should not) add any actual script runners. I don't want to just add and remove a normal script blocker because I want to a) assert immediately if someone tries to add a script runner when they shouldn't and b) if we accidentally screw up and add a script runner, I want that script runner to be dropped on the floor rather than executed at a time I know is unsafe. (Point b) is negotiable, you might argue that it's safer to execute the runner than drop it, but I definitely want the instant assertion.) This patch adds nsContentUtils::AddScriptBlockerAndPreventAddingRunners, which prevents adding any runners until that script blocker is removed.

Adds a script blocker while we call UpdateViewAfterScroll. Makes PluginWndProc WM_MOUSEACTIVATE bail out instead of dispatching an event if it's not safe to run script. Also suppresses FlushDelayedResize in nsViewManager::DispatchEvent NS_PAINT, if we're in the middle of scrolling --- this looks like another potential reentrancy hazard.

Comment on attachment 418773 [details] [diff] [review] Part 34: Create AddScriptBlockerAndPreventAddingRunners Wouldn't mind using a guard object here, though maybe that's overkill unless we end up using this in more places. Though sort of wondering if there's any place up the callstack where it would make sense to add a scriptblocker such that we could run script when we leave that scope?

(In reply to comment #74) > Though sort of wondering if there's any place up the callstack where it would > make sense to add a scriptblocker such that we could run script when we leave > that scope? The problem is that there are many different callstacks leading to nsIScrollableFrame::ScrollTo/ScrollBy, along which we hold frame pointers, and they would all need to be independently guarded with script blockers.

The first version of this patch didn't build on Windows, because nsPluginNativeWindowWin can't use nsContentUtils. But it turns out we actually don't need to change nsPluginNativeWindowWin. We don't do anything dangerous with WM_MOUSEACTIVATE except here in PluginWndProc where we call DispatchEvent with NS_PLUGIN_ACTIVATE. And that doesn't do anything dangerous before we get to PresShell::HandleEvent, which already bails out early if !IsSafeToRunScript.

I noticed a couple of test failures when pushing to the try server. We need to fix test_offsets.js to compute the scrolled area using floor, not round, and we also need to make nsFrame::FinishAndStoreOverflow handle empty overflow areas more precisely.

The nsFrame::FinishAndStoreOverflow changes are actually to avoid assertions about the scrolled area ending up smaller than the scrollport.

Comment on attachment 418808 [details] [diff] [review] Part 35 v2 >+ // Block script execution. This is basically a signal that flushing >+ // and handling of plugin events on Windows (WM_MOUSEACTIVATE) ... Fix comment now that you don't change the WM_MOUSEACTIVATE code? r=mats

http://hg.mozilla.org/mozilla-central/rev/5546b129745e http://hg.mozilla.org/mozilla-central/rev/db4c8c32177e http://hg.mozilla.org/mozilla-central/rev/eb1ae9aabecc http://hg.mozilla.org/mozilla-central/rev/8136d8a8d52f http://hg.mozilla.org/mozilla-central/rev/176699b95417 http://hg.mozilla.org/mozilla-central/rev/3d8d388c968b http://hg.mozilla.org/mozilla-central/rev/7448e55631cf http://hg.mozilla.org/mozilla-central/rev/8a224eb3aa04 http://hg.mozilla.org/mozilla-central/rev/a66a31bd68a9 http://hg.mozilla.org/mozilla-central/rev/78109c8760ca http://hg.mozilla.org/mozilla-central/rev/bbb0cb6172e3 http://hg.mozilla.org/mozilla-central/rev/c367c90a3853 http://hg.mozilla.org/mozilla-central/rev/bb8115d4556f http://hg.mozilla.org/mozilla-central/rev/a83834428689 http://hg.mozilla.org/mozilla-central/rev/927cce54b446 http://hg.mozilla.org/mozilla-central/rev/f1394b021aa0 http://hg.mozilla.org/mozilla-central/rev/7e30f976a6a3 http://hg.mozilla.org/mozilla-central/rev/8c1587eac4b2 http://hg.mozilla.org/mozilla-central/rev/2156590f92b2 http://hg.mozilla.org/mozilla-central/rev/2ec64a146a7f http://hg.mozilla.org/mozilla-central/rev/822e303a19a0 http://hg.mozilla.org/mozilla-central/rev/fa0d667648ce http://hg.mozilla.org/mozilla-central/rev/4fb067ad0ae4 http://hg.mozilla.org/mozilla-central/rev/b53b126bd64c http://hg.mozilla.org/mozilla-central/rev/49d5090912a4 http://hg.mozilla.org/mozilla-central/rev/5d8894904869 http://hg.mozilla.org/mozilla-central/rev/a423f5c3057a http://hg.mozilla.org/mozilla-central/rev/208d262ef05b http://hg.mozilla.org/mozilla-central/rev/d00058e431d3 http://hg.mozilla.org/mozilla-central/rev/fada8c5cef07 http://hg.mozilla.org/mozilla-central/rev/4ccff5df452c http://hg.mozilla.org/mozilla-central/rev/17131f693e28 http://hg.mozilla.org/mozilla-central/rev/4384150589e0 http://hg.mozilla.org/mozilla-central/rev/2f7d76044ee8 http://hg.mozilla.org/mozilla-central/rev/9584d6bce9c1 http://hg.mozilla.org/mozilla-central/rev/8544017aa583 http://hg.mozilla.org/mozilla-central/rev/47cd92d616d7 Thanks Mats!

Could this have caused the following bug? Alt-tabbing until the entire viewport is in "focus" and have a rectangle around it. Then scroll up/down. The focus rect's dotted line will smear. I found this happening beginning with the 2010-01-12 nightly builds. Previously, the focus rect would immediately disappear after scrolling, so no smearing is possible.

Yeah, it probably was this bug. Can you file it as a new bug and mark this bug as blocking it? Thanks.