RESOLVED FIXED 156225
[RTL Scrollbars] Overlay scrollbars push contents inwards
https://bugs.webkit.org/show_bug.cgi?id=156225
Summary [RTL Scrollbars] Overlay scrollbars push contents inwards
Myles C. Maxfield
Reported 2016-04-04 21:03:58 PDT
[RTL Scrollbars] Overlay scrollbars push contents inwards
Attachments
Patch (4.52 KB, patch)
2016-04-04 21:05 PDT, Myles C. Maxfield
no flags
Archive of layout-test-results from ews103 for mac-yosemite (983.02 KB, application/zip)
2016-04-04 21:50 PDT, Build Bot
no flags
Archive of layout-test-results from ews116 for mac-yosemite (867.70 KB, application/zip)
2016-04-04 22:03 PDT, Build Bot
no flags
Patch (4.49 KB, patch)
2016-04-11 22:18 PDT, Myles C. Maxfield
darin: review+
Myles C. Maxfield
Comment 1 2016-04-04 21:05:38 PDT
Myles C. Maxfield
Comment 2 2016-04-04 21:06:08 PDT
Build Bot
Comment 3 2016-04-04 21:50:25 PDT
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
Comment 4 2016-04-04 21:50:29 PDT
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 5 2016-04-04 22:03:42 PDT
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
Comment 6 2016-04-04 22:03:46 PDT
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 7 2016-04-05 11:54:08 PDT
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
Comment 8 2016-04-07 01:59:12 PDT
Unfortunately, I don't seem to be able to reproduce the crashes on Yosemite on my local machine.
Myles C. Maxfield
Comment 9 2016-04-07 15:07:12 PDT
I'm able to reproduce the crashes about once out of every 1500 runs
Darin Adler
Comment 10 2016-04-08 00:16:02 PDT
Comment on attachment 275632 [details] Patch Tests failing; I will review once EWS tests pass.
Myles C. Maxfield
Comment 11 2016-04-08 00:49:01 PDT
(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
Comment 12 2016-04-08 11:33:01 PDT
(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
Comment 13 2016-04-11 12:02:05 PDT
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
Comment 14 2016-04-11 12:28:45 PDT
(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
Comment 15 2016-04-11 12:36:47 PDT
(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
Comment 16 2016-04-11 12:44:14 PDT
(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
Comment 17 2016-04-11 12:56:45 PDT
(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
Comment 18 2016-04-11 22:18:20 PDT
Myles C. Maxfield
Comment 19 2016-04-12 11:40:14 PDT
Note You need to log in before you can comment on or make changes to this bug.