Bug 178710

Summary: RenderSVGModelObject::checkIntersection triggers layout
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: SVGAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, commit-queue, koivisto, rniwa, sabouhallawa, simon.fraser, zalan, zimmermann
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Fixes the bug
none
Address Simon's comment with a test
simon.fraser: review+
Patch for landing
buildbot: commit-queue-
Archive of layout-test-results from ews103 for mac-elcapitan
none
Archive of layout-test-results from ews116 for mac-elcapitan
none
Patch for landing none

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>