Bug 178715 - Style::Scope::flushPendingUpdate() can replace the entire document in XSLTProcessor::createDocumentFromSource
Summary: Style::Scope::flushPendingUpdate() can replace the entire document in XSLTPro...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
: 178938 (view as bug list)
Depends on:
Blocks: 178830 178845
  Show dependency treegraph
 
Reported: 2017-10-24 00:22 PDT by Ryosuke Niwa
Modified: 2019-02-06 09:03 PST (History)
14 users (show)

See Also:


Attachments
WIP (9.57 KB, patch)
2017-10-24 21:14 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Cleanup (12.68 KB, patch)
2017-10-25 13:18 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Patch for landing (12.74 KB, patch)
2017-10-25 15:46 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Fixed the flaky tests (14.04 KB, patch)
2017-10-28 01:15 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-24 00:22:06 PDT
3   0x3209ad7b8 WebCore::EventDispatcher::dispatchEvent(WebCore::Node&, WebCore::Event&)
4   0x321587d9d WebCore::Node::dispatchEvent(WebCore::Event&)
5   0x3207a3b2c WebCore::Document::setReadyState(WebCore::Document::ReadyState)
6   0x3207ae716 WebCore::Document::implicitOpen()
7   0x3207a4c76 WebCore::Document::open(WebCore::Document*)
8   0x3207a4a77 WebCore::Document::setContent(WTF::String const&)
9   0x3230dec83 WebCore::XSLTProcessor::createDocumentFromSource(WTF::String const&, WTF::String const&, WTF::String const&, WebCore::Node*, WebCore::Frame*)
10  0x3207bec53 WebCore::Document::applyXSLTransform(WebCore::ProcessingInstruction*)
11  0x321ddbd0e WebCore::Style::Scope::collectActiveStyleSheets(WTF::Vector<WTF::RefPtr<WebCore::StyleSheet>, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&)
12  0x321ddc981 WebCore::Style::Scope::updateActiveStyleSheets(WebCore::Style::Scope::UpdateType)
13  0x321dddbf3 WebCore::Style::Scope::flushPendingSelfUpdate()
14  0x320540042 WebCore::Style::Scope::flushPendingUpdate()
15  0x3207a9711 WebCore::Document::updateStyleIfNeeded()
16  0x3207c0363 WebCore::Document::finishedParsing()
17  0x323096212 WebCore::XMLDocumentParser::end()
18  0x32309630e WebCore::XMLDocumentParser::finish()
19  0x320873556 WebCore::DocumentWriter::end()
20  0x32082863f WebCore::DocumentLoader::finishedLoading()
21  0x32082837d WebCore::DocumentLoader::notifyFinished(WebCore::CachedResource&)
22  0x3208287cc non-virtual thunk to WebCore::DocumentLoader::notifyFinished(WebCore::CachedResource&)
Comment 1 Radar WebKit Bug Importer 2017-10-24 00:30:15 PDT
<rdar://problem/35144665>
Comment 2 Antti Koivisto 2017-10-24 00:49:38 PDT
XSLT stuff should be moved out of the style code. It is there for historical reasons.
Comment 3 Ryosuke Niwa 2017-10-24 01:22:18 PDT
(In reply to Antti Koivisto from comment #2)
> XSLT stuff should be moved out of the style code. It is there for historical
> reasons.

Yeah. I wonder if we can just apply it with a timer?
Comment 4 Ryosuke Niwa 2017-10-24 21:14:34 PDT
Created attachment 324791 [details]
WIP

Proposed approach:
1. Start a timer on document whenever XSL loads
2. Force applying XSLT whenever the document finishes loading to minimize the compat issues
Comment 5 Ryosuke Niwa 2017-10-25 13:18:25 PDT
Created attachment 324877 [details]
Cleanup
Comment 6 Ryosuke Niwa 2017-10-25 14:07:05 PDT
Comment on attachment 324877 [details]
Cleanup

View in context: https://bugs.webkit.org/attachment.cgi?id=324877&action=review

> Source/WebCore/ChangeLog:30
> +        (WebCore::Style::Scope::collectXLSTransforms): Added.

Ugh... there's a typo here.
Comment 7 Brent Fulgham 2017-10-25 14:13:09 PDT
Comment on attachment 324877 [details]
Cleanup

View in context: https://bugs.webkit.org/attachment.cgi?id=324877&action=review

Looks good! r=me.

> Source/WebCore/dom/Document.cpp:5082
> +        RefPtr<XSLTProcessor> processor = XSLTProcessor::create();

auto?

> Source/WebCore/style/StyleScope.cpp:294
> +// FIXME: Remove the code related to XSLT from Style::Scope.

Can you file a bug (or Radar) so we don't forget to do this?
Comment 8 Ryosuke Niwa 2017-10-25 14:18:52 PDT
(In reply to Brent Fulgham from comment #7)
> Comment on attachment 324877 [details]
> Cleanup
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=324877&action=review
> 
> Looks good! r=me.
> 
> > Source/WebCore/dom/Document.cpp:5082
> > +        RefPtr<XSLTProcessor> processor = XSLTProcessor::create();
> 
> auto?

Fixed.

> > Source/WebCore/style/StyleScope.cpp:294
> > +// FIXME: Remove the code related to XSLT from Style::Scope.
> 
> Can you file a bug (or Radar) so we don't forget to do this?

Filed webkit.org/b/178830
Comment 9 David Kilzer (:ddkilzer) 2017-10-25 14:27:42 PDT
Comment on attachment 324877 [details]
Cleanup

View in context: https://bugs.webkit.org/attachment.cgi?id=324877&action=review

For what (little) I know about style resolution, this seems fine.
You should have an expert like Antti review, though.

> Source/WebCore/dom/Document.cpp:5067
> +void Document::applyPendingXSLTransformsNowIfScheduled()
> +{
> +    if (m_applyPendingXSLTransformsTimer.isActive())
> +        applyPendingXSLTransformsTimerFired();
> +}

Shouldn't you cancel the timer if it's active here?  It probably won't hurt anything if fired later, but it's work that can be avoided.
Comment 10 Ryosuke Niwa 2017-10-25 14:28:14 PDT
(In reply to David Kilzer (:ddkilzer) from comment #9)
> Comment on attachment 324877 [details]
> Cleanup
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=324877&action=review
> 
> For what (little) I know about style resolution, this seems fine.
> You should have an expert like Antti review, though.
> 
> > Source/WebCore/dom/Document.cpp:5067
> > +void Document::applyPendingXSLTransformsNowIfScheduled()
> > +{
> > +    if (m_applyPendingXSLTransformsTimer.isActive())
> > +        applyPendingXSLTransformsTimerFired();
> > +}
> 
> Shouldn't you cancel the timer if it's active here?  It probably won't hurt
> anything if fired later, but it's work that can be avoided.

Oh, that's a good point. Will fix.
Comment 11 Ryosuke Niwa 2017-10-25 15:46:44 PDT
Created attachment 324911 [details]
Patch for landing
Comment 12 Ryosuke Niwa 2017-10-25 15:47:04 PDT
Comment on attachment 324911 [details]
Patch for landing

Wait for EWS again.
Comment 13 Ryosuke Niwa 2017-10-25 17:30:05 PDT
Committed r223999: <https://trac.webkit.org/changeset/223999>
Comment 14 Alexey Proskuryakov 2017-10-25 18:30:36 PDT
Should we be worried at all about what happens before the timer fires? For example, what if something else removes the document from frame?
Comment 15 Ryosuke Niwa 2017-10-25 19:27:26 PDT
(In reply to Alexey Proskuryakov from comment #14)
> Should we be worried at all about what happens before the timer fires? For
> example, what if something else removes the document from frame?

XSLTProcessor::createDocumentFromSource has a check for frame() ands doesn't replace document when the frame had been detached (i.e. when frame() returns nullptr in Document::applyPendingXSLTransformsTimerFired)
Comment 16 Ryan Haddad 2017-10-27 10:01:04 PDT
(In reply to Ryosuke Niwa from comment #13)
> Committed r223999: <https://trac.webkit.org/changeset/223999>
This change caused multiple xsl LayoutTests to become flaky. See https://bugs.webkit.org/show_bug.cgi?id=178938
Comment 17 Ryan Haddad 2017-10-27 10:06:22 PDT
Reverted r223999 for reason:

Caused xsl LayoutTest flakiness.

Committed r224116: <https://trac.webkit.org/changeset/224116>
Comment 18 Ryan Haddad 2017-10-27 10:06:48 PDT
*** Bug 178938 has been marked as a duplicate of this bug. ***
Comment 19 Don Olmstead 2017-10-27 11:38:38 PDT
When you get around to relanding this please make sure to add guards for ports not enabling XSLT. See https://bugs.webkit.org/show_bug.cgi?id=178930
Comment 20 Ryosuke Niwa 2017-10-28 01:15:29 PDT
Created attachment 325253 [details]
Fixed the flaky tests
Comment 21 WebKit Commit Bot 2017-10-28 02:18:03 PDT
Comment on attachment 325253 [details]
Fixed the flaky tests

Clearing flags on attachment: 325253

Committed r224146: <https://trac.webkit.org/changeset/224146>
Comment 22 WebKit Commit Bot 2017-10-28 02:18:05 PDT
All reviewed patches have been landed.  Closing bug.
Comment 23 Ryosuke Niwa 2017-10-28 02:51:50 PDT
For posterity, the flakiness was only observable on release builds of WebKit2 on my 15" MBP.

The problem was that when m_applyPendingXSLTransformsTimer fires while parsing() is true, applyPendingXSLTransformsTimerFired would exit early without applying XLST. A subsequent call to applyPendingXSLTransformsNowIfScheduled would check !m_applyPendingXSLTransformsTimer.isActive() and exit early.

The flakiness was fixed by adding a boolean flag m_hasPendingXSLTransforms which is set whenever scheduleToApplyXSLTransforms is called instead of relying on m_applyPendingXSLTransformsTimer.isActive() being true in applyPendingXSLTransformsNowIfScheduled instead.

Because this race condition only happens when parsing() is true, finishParsing() would eventually call applyPendingXSLTransformsNowIfScheduled. As such, as we don't have to schedule a new timer there.

The latest patch also fixed the problem that Document::suspendScheduledTasks and Document::resumeScheduledTasks weren't suspending & resuming m_hasPendingXSLTransforms.
Comment 24 Lucas Forschler 2019-02-06 09:03:20 PST
Mass moving XML DOM bugs to the "DOM" Component.