Bug 178710 - RenderSVGModelObject::checkIntersection triggers layout
Summary: RenderSVGModelObject::checkIntersection triggers layout
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-10-23 22:49 PDT by Ryosuke Niwa
Modified: 2017-10-24 18:35 PDT (History)
8 users (show)

See Also:


Attachments
Fixes the bug (2.14 KB, patch)
2017-10-23 22:50 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Address Simon's comment with a test (17.18 KB, patch)
2017-10-24 14:22 PDT, Ryosuke Niwa
simon.fraser: review+
Details | Formatted Diff | Diff
Patch for landing (17.19 KB, patch)
2017-10-24 14:53 PDT, Ryosuke Niwa
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-elcapitan (1.18 MB, application/zip)
2017-10-24 16:11 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews116 for mac-elcapitan (1.77 MB, application/zip)
2017-10-24 16:16 PDT, Build Bot
no flags Details
Patch for landing (17.17 KB, patch)
2017-10-24 16:20 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2017-10-23 22:49:20 PDT
Don't do that.
Comment 1 Ryosuke Niwa 2017-10-23 22:50:58 PDT
Created attachment 324646 [details]
Fixes the bug
Comment 2 Ryosuke Niwa 2017-10-23 22:51:14 PDT
<rdar://problem/35140862>
Comment 3 Ryosuke Niwa 2017-10-23 22:54:34 PDT
In particular, collectIntersectionOrEnclosureList uses ElementIterator so it's not safe to update the layout which could run arbitrary scripts at the moment...
Comment 4 WebKit Commit Bot 2017-10-24 00:41:43 PDT
Comment on attachment 324646 [details]
Fixes the bug

Clearing flags on attachment: 324646

Committed r223882: <https://trac.webkit.org/changeset/223882>
Comment 5 WebKit Commit Bot 2017-10-24 00:41:44 PDT
All reviewed patches have been landed.  Closing bug.
Comment 6 Simon Fraser (smfr) 2017-10-24 10:31:45 PDT
Comment on attachment 324646 [details]
Fixes the bug

This doesn't seem right. getElementCTM() can be called from RenderSVGModelObject::checkIntersection, which is called from SVGSVGElement::checkIntersection which is exposed to JS. So now nothing forces layout before checkIntersection calls getElementCTM.
Comment 7 Ryosuke Niwa 2017-10-24 10:33:05 PDT
(In reply to Simon Fraser (smfr) from comment #6)
> Comment on attachment 324646 [details]
> Fixes the bug
> 
> This doesn't seem right. getElementCTM() can be called from
> RenderSVGModelObject::checkIntersection, which is called from
> SVGSVGElement::checkIntersection which is exposed to JS. So now nothing
> forces layout before checkIntersection calls getElementCTM.

SVGSVGElement::checkIntersection triggers layout!
https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/svg/SVGSVGElement.cpp#L336
Comment 8 Simon Fraser (smfr) 2017-10-24 10:34:19 PDT
(In reply to Ryosuke Niwa from comment #7)
> (In reply to Simon Fraser (smfr) from comment #6)
> > Comment on attachment 324646 [details]
> > Fixes the bug
> > 
> > This doesn't seem right. getElementCTM() can be called from
> > RenderSVGModelObject::checkIntersection, which is called from
> > SVGSVGElement::checkIntersection which is exposed to JS. So now nothing
> > forces layout before checkIntersection calls getElementCTM.
> 
> SVGSVGElement::checkIntersection triggers layout!
> https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/svg/
> SVGSVGElement.cpp#L336

That's getIntersectionList, not checkIntersection.
Comment 9 Ryosuke Niwa 2017-10-24 10:35:53 PDT
(In reply to Simon Fraser (smfr) from comment #8)
> (In reply to Ryosuke Niwa from comment #7)
> > (In reply to Simon Fraser (smfr) from comment #6)
> > > Comment on attachment 324646 [details]
> > > Fixes the bug
> > > 
> > > This doesn't seem right. getElementCTM() can be called from
> > > RenderSVGModelObject::checkIntersection, which is called from
> > > SVGSVGElement::checkIntersection which is exposed to JS. So now nothing
> > > forces layout before checkIntersection calls getElementCTM.
> > 
> > SVGSVGElement::checkIntersection triggers layout!
> > https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/svg/
> > SVGSVGElement.cpp#L336
> 
> That's getIntersectionList, not checkIntersection.

Oh, didn't notice those were exposed directly. Let's fix that...
Comment 10 Ryosuke Niwa 2017-10-24 10:36:10 PDT
Re-opening to address Simon's comment.
Comment 11 Ryosuke Niwa 2017-10-24 14:22:36 PDT
Created attachment 324720 [details]
Address Simon's comment with a test
Comment 12 Ryosuke Niwa 2017-10-24 14:51:39 PDT
Comment on attachment 324720 [details]
Address Simon's comment with a test

Posting a new patch to land.
Comment 13 Ryosuke Niwa 2017-10-24 14:53:20 PDT
Created attachment 324727 [details]
Patch for landing
Comment 14 Build Bot 2017-10-24 16:11:23 PDT
Comment on attachment 324727 [details]
Patch for landing

Attachment 324727 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4975689

New failing tests:
storage/indexeddb/detached-iframe.html
Comment 15 Build Bot 2017-10-24 16:11:24 PDT
Created attachment 324744 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 16 Build Bot 2017-10-24 16:16:41 PDT
Comment on attachment 324727 [details]
Patch for landing

Attachment 324727 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4975525

New failing tests:
storage/indexeddb/detached-iframe.html
Comment 17 Build Bot 2017-10-24 16:16:42 PDT
Created attachment 324745 [details]
Archive of layout-test-results from ews116 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 18 Ryosuke Niwa 2017-10-24 16:20:50 PDT
Created attachment 324746 [details]
Patch for landing
Comment 19 Ryosuke Niwa 2017-10-24 18:35:47 PDT
Committed r223947: <https://trac.webkit.org/changeset/223947>