WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
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
Details
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
Details
Patch
(4.49 KB, patch)
2016-04-11 22:18 PDT
,
Myles C. Maxfield
darin
: review+
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Myles C. Maxfield
Comment 1
2016-04-04 21:05:38 PDT
Created
attachment 275632
[details]
Patch
Myles C. Maxfield
Comment 2
2016-04-04 21:06:08 PDT
<
rdar://problem/25137040
>
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
Created
attachment 276220
[details]
Patch
Myles C. Maxfield
Comment 19
2016-04-12 11:40:14 PDT
Committed
r199377
: <
http://trac.webkit.org/changeset/199377
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug