| Summary: | Crash under Page::scrollingCoordinator() | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Antoine Quint <graouts> | ||||
| Component: | Layout and Rendering | Assignee: | Antoine Quint <graouts> | ||||
| Status: | NEW --- | ||||||
| Severity: | Normal | CC: | bfulgham, dino, simon.fraser, webkit-bug-importer, zalan | ||||
| Priority: | P2 | Keywords: | InRadar | ||||
| Version: | Safari 11 | ||||||
| Hardware: | Unspecified | ||||||
| OS: | macOS 10.13 | ||||||
| Attachments: |
|
||||||
|
Description
Antoine Quint
2018-06-07 06:47:46 PDT
ScrollingCoordinator* Page::scrollingCoordinator()
{
if (!m_scrollingCoordinator && m_settings->scrollingCoordinatorEnabled()) {
m_scrollingCoordinator = chrome().client().createScrollingCoordinator(*this);
if (!m_scrollingCoordinator)
m_scrollingCoordinator = ScrollingCoordinator::create(this);
}
return m_scrollingCoordinator.get();
}
I suppose we could crash if either m_settings is nullptr or if m_scrollingCoordinator is nullptr and m_settings->scrollingCoordinatorEnabled() is false.
Created attachment 342156 [details]
Patch
Comment on attachment 342156 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=342156&action=review > Source/WebCore/page/Page.cpp:377 > + if (!m_settings || !m_settings->scrollingCoordinatorEnabled()) > + return nullptr; m_settings can't really be nullptr and as long as the Page::settings() returns Settings& we should not add random nullptr checks (since the contract is that as long as the page is valid, the settings is valid too ) Not sure way we would crash with m_scrollingCoordinator nullptr. If we end up not constructing a scrollingCoordinator object, m_scrollingCoordinator.get() would just return nullptr. Also I think this stacktrace is about accessing an invalid Page object. But the call site in ~RenderLayer() accesses the Page through renderer().page().scrollingCoordinator() and this should return a Page&, so this ought not be null either. (In reply to Antoine Quint from comment #5) > But the call site in ~RenderLayer() accesses the Page through > renderer().page().scrollingCoordinator() and this should return a Page&, so > this ought not be null either. Sure and if it turns out to be null, then we need to find out why and assess whether it's a valid case and either change the contract (Page& -> Page*) or fix the broken case. (In reply to zalan from comment #6) > (In reply to Antoine Quint from comment #5) > > But the call site in ~RenderLayer() accesses the Page through > > renderer().page().scrollingCoordinator() and this should return a Page&, so > > this ought not be null either. > Sure and if it turns out to be null, then we need to find out why and assess > whether it's a valid case and either change the contract (Page& -> Page*) or > fix the broken case. My point is that the combination of null checks and returning the reference is confusing and should be avoided. it's either guaranteed to be not null -> &, or null -> * (there are obviously some exceptions but I don't think this falls into that category) |