[RTL Scrollbars] Overlay scrollbars push contents inwards
Created attachment 275632 [details] Patch
<rdar://problem/25137040>
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
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
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
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
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?
Unfortunately, I don't seem to be able to reproduce the crashes on Yosemite on my local machine.
I'm able to reproduce the crashes about once out of every 1500 runs
Comment on attachment 275632 [details] Patch Tests failing; I will review once EWS tests pass.
(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.
(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.
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.
(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)
(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)
(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
(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.
Created attachment 276220 [details] Patch
Committed r199377: <http://trac.webkit.org/changeset/199377>