Bug 56067 - Overlay scrollers in overflow areas need send notifications at appropriate times (showing up, resizing)
Summary: Overlay scrollers in overflow areas need send notifications at appropriate ti...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Beth Dakin
URL:
Keywords: InRadar
Depends on: 56163
Blocks:
  Show dependency treegraph
 
Reported: 2011-03-09 18:14 PST by Beth Dakin
Modified: 2012-02-03 08:48 PST (History)
11 users (show)

See Also:


Attachments
Patch (15.93 KB, patch)
2011-03-09 18:14 PST, Beth Dakin
darin: review+
Details | Formatted Diff | Diff
Better Patch (20.00 KB, patch)
2011-03-11 10:22 PST, Beth Dakin
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Beth Dakin 2011-03-09 18:14:02 PST
The view's scrollbars send notifications through the ScrollAnimator for things like painting, resizing, etc. Scrollbars for overflow areas need to do the same.

<rdar://problem/8944558>
Comment 1 Beth Dakin 2011-03-09 18:14:53 PST
Created attachment 85271 [details]
Patch
Comment 2 Simon Fraser (smfr) 2011-03-09 18:27:45 PST
Comment on attachment 85271 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=85271&action=review

> Source/WebCore/page/FocusController.cpp:416
> +        HashSet<ScrollableArea*>::iterator end = scrollableAreas->end(); 
> +        for (HashSet<ScrollableArea*>::iterator it = scrollableAreas->begin(); it != end; ++it) {

These could be const_iterators.

> Source/WebCore/page/FrameView.cpp:2063
> +    HashSet<ScrollableArea*>::iterator end = scrollableAreas->end(); 
> +    for (HashSet<ScrollableArea*>::iterator it = scrollableAreas->begin(); it != end; ++it)

Ditto.
Comment 3 Beth Dakin 2011-03-09 18:31:41 PST
(In reply to comment #2)
> (From update of attachment 85271 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=85271&action=review
> 
> > Source/WebCore/page/FocusController.cpp:416
> > +        HashSet<ScrollableArea*>::iterator end = scrollableAreas->end(); 
> > +        for (HashSet<ScrollableArea*>::iterator it = scrollableAreas->begin(); it != end; ++it) {
> 
> These could be const_iterators.
> 
> > Source/WebCore/page/FrameView.cpp:2063
> > +    HashSet<ScrollableArea*>::iterator end = scrollableAreas->end(); 
> > +    for (HashSet<ScrollableArea*>::iterator it = scrollableAreas->begin(); it != end; ++it)
> 
> Ditto.

Fixed!
Comment 4 Darin Adler 2011-03-10 16:58:30 PST
Comment on attachment 85271 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=85271&action=review

> Source/WebCore/page/EventHandler.cpp:1873
> +        RenderLayer* layerForLastNode = m_lastNodeUnderMouse && m_lastNodeUnderMouse->renderer()
> +                                        ? m_lastNodeUnderMouse->renderer()->enclosingLayer() : 0;
> +        RenderLayer* layerForNodeUnderMouse = m_nodeUnderMouse && m_nodeUnderMouse->renderer()
> +                                        ? m_nodeUnderMouse->renderer()->enclosingLayer() : 0;

I’d like to use a helper function to get the layer for a node so we don’t have these unpleasant ? : expressions. That helper could also be used in EventHandler::mouseMoved.

> Source/WebCore/page/EventHandler.cpp:1877
> +            // The mouse has moved between frames

We normally use periods in this kind of comment.

> Source/WebCore/page/FocusController.cpp:414
> +        HashSet<ScrollableArea*>* scrollableAreas = m_page->scrollableAreaSet();

Missing check for null.

> Source/WebCore/page/FrameView.cpp:2061
> +    HashSet<ScrollableArea*>* scrollableAreas = page->scrollableAreaSet();

Missing check for null.

> Source/WebCore/page/Page.cpp:898
> +        m_scrollableAreaSet = new ScrollableAreaSet;

Please use adoptPtr here.

> Source/WebCore/page/Page.h:286
> +        bool pageContainsScrollableArea(ScrollableArea*) const;

Don’t need the word page in this function name. The class is Page.

> Source/WebCore/page/Page.h:287
> +        ScrollableAreaSet* scrollableAreaSet() const { return m_scrollableAreaSet.get(); }

This should probably return a const ScrollableAreaSet* since you only use it to iterate all the areas.
Comment 5 Beth Dakin 2011-03-10 16:59:52 PST
Thanks Darin! I will address all of your comments before checking in.
Comment 6 Beth Dakin 2011-03-10 17:54:09 PST
Fixed with revision 80800.
Comment 8 WebKit Review Bot 2011-03-10 18:55:50 PST
http://trac.webkit.org/changeset/80800 might have broken Qt Linux Release
The following tests are not passing:
editing/selection/4975120.html
editing/selection/caret-and-focus-ring.html
fast/dom/Window/window-onFocus.html
fast/events/blur-focus-window-should-blur-focus-element.html
fast/events/show-modal-dialog-onblur-onfocus.html
fast/repaint/fixed-move-after-keyboard-scroll.html
plugins/netscape-plugin-setwindow-size-2.html
Comment 9 Ryosuke Niwa 2011-03-10 19:04:47 PST
I rolled it out in http://trac.webkit.org/changeset/80804 for now.
Comment 11 Adam Roben (:aroben) 2011-03-10 19:39:46 PST
Looks like it also caused crashes in FocusController::setActive on Leopard: http://build.webkit.org/results/Leopard%20Intel%20Debug%20(Tests)/r80803%20(27453)/results.html
Comment 12 Adam Roben (:aroben) 2011-03-10 19:41:12 PST
The Windows crash log say a null dereference is happening here:

FAULTING_SOURCE_CODE:  
  2064:         return;
  2065: 
  2066:     HashSet<ScrollableArea*>::const_iterator end = scrollableAreas->end(); 
  2067:     for (HashSet<ScrollableArea*>::const_iterator it = scrollableAreas->begin(); it != end; ++it)
> 2068:         (*it)->scrollAnimator()->contentAreaWillPaint();
  2069: }
  2070: 
  2071: #if ENABLE(DASHBOARD_SUPPORT)
  2072: void FrameView::updateDashboardRegions()
  2073: {

I don't know whether *it was null or scrollAnimator() was null.
Comment 13 Beth Dakin 2011-03-11 10:22:06 PST
Created attachment 85488 [details]
Better Patch

Sorry about the crashes. The crashes happen on MacOS X too, and I was able to find a reproducible case. I discovered that when we crashed, the ScrollableArea was garbage. The problem is that we added the ScrollableArea, and were not able to remove it because when the ScrollableArea was destroyed, we no longer had a reference to the Page. Here is a new patch that adds a member variable to the relevant ScrollableArea classes to keep a weak pointer to Page so that they don't have to rely on Frames and/or Renderers having Pages still. This patch fixes the reproducible case that I found. 

However, I do not have time to give this patch the testing that it needs today, so I do not intend to check it in until I live on it more. I will update the bug once I have done that.

In the meantime, if anyone has comments on the approach or the patch in general, please comment :-)
Comment 14 WebKit Review Bot 2011-03-11 10:26:22 PST
Attachment 85488 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/page/Page.cpp:198:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 15 Beth Dakin 2011-03-11 12:01:20 PST
Oops, meant to re-open this when I posted the new patch.
Comment 16 Darin Adler 2011-03-12 07:58:46 PST
Comment on attachment 85488 [details]
Better Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=85488&action=review

Not sure why the patch is not applying on some platforms.

>> Source/WebCore/page/Page.cpp:198
>> +        }
> 
> One line control clauses should not use braces.  [whitespace/braces] [4]

I agree with the style bot on this.
Comment 17 Beth Dakin 2011-03-15 19:22:21 PDT
Patch committed with revision 81209.
Comment 18 WebKit Review Bot 2011-03-16 09:58:11 PDT
http://trac.webkit.org/changeset/81209 might have broken GTK Linux 32-bit Release
Comment 19 Yong Li 2012-02-02 14:39:51 PST
(In reply to comment #13)
> Created an attachment (id=85488) [details]
> Better Patch
> 
> Sorry about the crashes. The crashes happen on MacOS X too, and I was able to find a reproducible case. I discovered that when we crashed, the ScrollableArea was garbage. The problem is that we added the ScrollableArea, and were not able to remove it because when the ScrollableArea was destroyed, we no longer had a reference to the Page. Here is a new patch that adds a member variable to the relevant ScrollableArea classes to keep a weak pointer to Page so that they don't have to rely on Frames and/or Renderers having Pages still. This patch fixes the reproducible case that I found. 
> 
> However, I do not have time to give this patch the testing that it needs today, so I do not intend to check it in until I live on it more. I will update the bug once I have done that.
> 
> In the meantime, if anyone has comments on the approach or the patch in general, please comment :-)

m_page seems dangerous and redundant. Is it safe to remove the ScrollableArea through Frame::detachFromPage or FrameLoader::detachFromParent so we can remove m_page?
Comment 20 Antonio Gomes 2012-02-03 08:48:30 PST
(In reply to comment #19)
> (In reply to comment #13)
> > Created an attachment (id=85488) [details] [details]
> > Better Patch
> > 
> > Sorry about the crashes. The crashes happen on MacOS X too, and I was able to find a reproducible case. I discovered that when we crashed, the ScrollableArea was garbage. The problem is that we added the ScrollableArea, and were not able to remove it because when the ScrollableArea was destroyed, we no longer had a reference to the Page. Here is a new patch that adds a member variable to the relevant ScrollableArea classes to keep a weak pointer to Page so that they don't have to rely on Frames and/or Renderers having Pages still. This patch fixes the reproducible case that I found. 
> > 
> > However, I do not have time to give this patch the testing that it needs today, so I do not intend to check it in until I live on it more. I will update the bug once I have done that.
> > 
> > In the meantime, if anyone has comments on the approach or the patch in general, please comment :-)
> 
> m_page seems dangerous and redundant. Is it safe to remove the ScrollableArea through Frame::detachFromPage or FrameLoader::detachFromParent so we can remove m_page?

Beth, we are seeing issues where the cached   m_page  (named weak, but in fact just a raw point to page) is cause crashes, likely in middle of its destruction or in rare cases.

I think Yong's question is pertinent, so we avoid the m_page cache at all. What do you think?