Bug 156225 – [RTL Scrollbars] Overlay scrollbars push contents inwards

• Component: New Bugs
• Status: RESOLVED FIXED

[Myles C. Maxfield]

[RTL Scrollbars] Overlay scrollbars push contents inwards

[Myles C. Maxfield]

Created attachment 275632 [details]
Patch

[Myles C. Maxfield]

<rdar://problem/25137040>

[Build Bot]

Comment on attachment 275632 [details]
Patch

Attachment 275632 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1101453

New failing tests:
fast/scrolling/rtl-scrollbars-overlay-no-push-contents.html

[Build Bot]

Created attachment 275635 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 275632 [details]
Patch

Attachment 275632 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1101466

New failing tests:
fast/scrolling/rtl-scrollbars-overlay-no-push-contents.html

[Build Bot]

Created attachment 275639 [details]
Archive of layout-test-results from ews116 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Simon Fraser (smfr)]

Comment on attachment 275632 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        [RTL Scrollbars] Overlay scrollbars push contents inwards

From the title it's not clear if this is a "push contents and should not", or "need to push contents". Please clarify.

> Source/WebCore/ChangeLog:10
> +        The contents should be pushed in by the occupied width of the
> +        scrollbar, which is 0 for overlay scrollbars.

And what happens before the patch?

[Myles C. Maxfield]

Unfortunately, I don't seem to be able to reproduce the crashes on Yosemite on my local machine.

[Myles C. Maxfield]

I'm able to reproduce the crashes about once out of every 1500 runs

[Darin Adler]

Comment on attachment 275632 [details]
Patch

Tests failing; I will review once EWS tests pass.

[Myles C. Maxfield]

(In reply to comment #9)
> I'm able to reproduce the crashes about once out of every 1500 runs

.... But only on Yosemite. On El Capitan, the test ran successfully 9000 times in a row.

[Myles C. Maxfield]

(In reply to comment #11)
> (In reply to comment #9)
> > I'm able to reproduce the crashes about once out of every 1500 runs
> 
> .... But only on Yosemite. On El Capitan, the test ran successfully 9000
> times in a row.

.... But not on my work machine running Yosemite. There, the test successfully ran 29000 times (overnight) in a row.

[Myles C. Maxfield]

We're not clearing the NSScrollerImp's delegate pointer because when we look for the NSScrollerImp (by calling scrollerImpForScrollbar(*scrollbar) inside ) we're coming up empty. Therefore, the delegate pointer never gets cleared, and AppKit will follow it and crash.

[Myles C. Maxfield]

(In reply to comment #13)
> We're not clearing the NSScrollerImp's delegate pointer because when we look
> for the NSScrollerImp (by calling scrollerImpForScrollbar(*scrollbar) inside
> ) we're coming up empty. Therefore, the delegate pointer never gets cleared,
> and AppKit will follow it and crash.

This is because mock scrollbars get re-enabled before we tear down the web view, which means the destruction doesn't match the construction (so the reregistration won't work appropriately)

Mock scrollbars get re-enabled because DRT's runTest() function calls gTestRunner->setDeveloperExtrasEnabled(false); which causes us to re-apply our preferences (and the JS call to stop mocking the scrollbars doesn't change the preference)

[Myles C. Maxfield]

(In reply to comment #14)
> (In reply to comment #13)
> > We're not clearing the NSScrollerImp's delegate pointer because when we look
> > for the NSScrollerImp (by calling scrollerImpForScrollbar(*scrollbar) inside
> > ) we're coming up empty. Therefore, the delegate pointer never gets cleared,
> > and AppKit will follow it and crash.
> 
> This is because mock scrollbars get re-enabled before we tear down the web
> view, which means the destruction doesn't match the construction (so the
> reregistration won't work appropriately)
> 
> Mock scrollbars get re-enabled because DRT's runTest() function calls
> gTestRunner->setDeveloperExtrasEnabled(false); which causes us to re-apply
> our preferences (and the JS call to stop mocking the scrollbars doesn't
> change the preference)

Quickly hacking up the mock scrollbar flag to ignore that re-enable causes the crash to be avoided. (This verifies the theory)

[Myles C. Maxfield]

(In reply to comment #15)
> (In reply to comment #14)
> > (In reply to comment #13)
> > > We're not clearing the NSScrollerImp's delegate pointer because when we look
> > > for the NSScrollerImp (by calling scrollerImpForScrollbar(*scrollbar) inside
> > > ) we're coming up empty. Therefore, the delegate pointer never gets cleared,
> > > and AppKit will follow it and crash.
> > 
> > This is because mock scrollbars get re-enabled before we tear down the web
> > view, which means the destruction doesn't match the construction (so the
> > reregistration won't work appropriately)
> > 
> > Mock scrollbars get re-enabled because DRT's runTest() function calls
> > gTestRunner->setDeveloperExtrasEnabled(false); which causes us to re-apply
> > our preferences (and the JS call to stop mocking the scrollbars doesn't
> > change the preference)
> 
> Quickly hacking up the mock scrollbar flag to ignore that re-enable causes
> the crash to be avoided. (This verifies the theory)

The relevant preferences object is of type WebPreferences and is gotten from WebFrame -> webView -> preferences

[Myles C. Maxfield]

(In reply to comment #16)
> (In reply to comment #15)
> > (In reply to comment #14)
> > > (In reply to comment #13)
> > > > We're not clearing the NSScrollerImp's delegate pointer because when we look
> > > > for the NSScrollerImp (by calling scrollerImpForScrollbar(*scrollbar) inside
> > > > ) we're coming up empty. Therefore, the delegate pointer never gets cleared,
> > > > and AppKit will follow it and crash.
> > > 
> > > This is because mock scrollbars get re-enabled before we tear down the web
> > > view, which means the destruction doesn't match the construction (so the
> > > reregistration won't work appropriately)
> > > 
> > > Mock scrollbars get re-enabled because DRT's runTest() function calls
> > > gTestRunner->setDeveloperExtrasEnabled(false); which causes us to re-apply
> > > our preferences (and the JS call to stop mocking the scrollbars doesn't
> > > change the preference)
> > 
> > Quickly hacking up the mock scrollbar flag to ignore that re-enable causes
> > the crash to be avoided. (This verifies the theory)
> 
> The relevant preferences object is of type WebPreferences and is gotten from
> WebFrame -> webView -> preferences

DRT says explicitly:

// The mock scrollbars setting cannot be modified after creating a view, so we have to do it now.

[Myles C. Maxfield]

Created attachment 276220 [details]
Patch

[Myles C. Maxfield]

Committed r199377: <http://trac.webkit.org/changeset/199377>