[Webkit-unassigned] [Bug 184366] crash when destroying a RenderObject with orca running

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sat Apr 14 13:03:15 PDT 2018


https://bugs.webkit.org/show_bug.cgi?id=184366

--- Comment #7 from Ryosuke Niwa <rniwa at webkit.org> ---
(In reply to Mike Gorse from comment #6)
> (In reply to Ryosuke Niwa from comment #5)
> > (In reply to Mike Gorse from comment #4)
> > > Created attachment 337704 [details]
> > > Patch.
> > > 
> > > I'm submitting the attached patch for openSUSE. It's a partial revert of the
> > > commit from bug 182513. I'm not sure if this should be committed as-is, but
> > > it fixes the crash for me and shouldn't cause any functional change outside
> > > of accessibility.
> > 
> > This change simply removes the release assertion. We need to address the
> > underlying issue which is that accessibility code in GTK+ port is updating
> > layout in the middle of deleting render objects. That's never safe, and can
> > lead to memory corruption. This crash is currently protecting you from
> > having an exploitable security bug.
> 
> I didn't remove any assert lines, but I changed the condition under which
> document->updateLayoutIgnorePendingStyleSheets is called, so I don't
> understand your comment. I altered a couple of asserts along with changing a
> prototype. That being said, I'm not necessarily arguing that what I attached
> is what should be committed.

Checking isSafeToUpdateStyleOrLayout is not enough. That function return true for some cases where it shouldn't like when frame flattening is enabled, etc...

Fundamentally, the fix needs to specifically detect the case where this function called while render objects are being destructed, and defer the work including but not limited to updating layout. The patch you posted, for example, would yield a wrong result because the layout is not updated in some cases now; but it still updates layout when it shouldn't (because isSafeToUpdateStyleOrLayout returns true in some cases where we had to add exceptions to make WebKit not too crashy).

In general, the fix for these kinds of crashes involving isSafeToUpdateStyleOrLayout should not check the return value of isSafeToUpdateStyleOrLayout. That's an anti-pattern, and fundamentally flawed because updating layout is either always needed or always unnecessary. Just because it's not safe to update the layout, it doesn't mean it's not needed.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20180414/3baa47e1/attachment-0001.html>


More information about the webkit-unassigned mailing list