Bug 178715 – Style::Scope::flushPendingUpdate() can replace the entire document in XSLTProcessor::createDocumentFromSource

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Ryosuke Niwa]

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&)

[Radar WebKit Bug Importer]

<rdar://problem/35144665>

[Antti Koivisto]

XSLT stuff should be moved out of the style code. It is there for historical reasons.

[Ryosuke Niwa]

(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?

[Ryosuke Niwa]

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

[Ryosuke Niwa]

Created attachment 324877 [details]
Cleanup

[Ryosuke Niwa]

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.

[Brent Fulgham]

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?

[Ryosuke Niwa]

(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

[David Kilzer (:ddkilzer)]

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.

[Ryosuke Niwa]

(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.

[Ryosuke Niwa]

Created attachment 324911 [details]
Patch for landing

[Ryosuke Niwa]

Comment on attachment 324911 [details]
Patch for landing

Wait for EWS again.

[Ryosuke Niwa]

Committed r223999: <https://trac.webkit.org/changeset/223999>

[Alexey Proskuryakov]

Should we be worried at all about what happens before the timer fires? For example, what if something else removes the document from frame?

[Ryosuke Niwa]

(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)

[Ryan Haddad]

(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

[Ryan Haddad]

Reverted r223999 for reason:

Caused xsl LayoutTest flakiness.

Committed r224116: <https://trac.webkit.org/changeset/224116>

[Ryan Haddad]

*** Bug 178938 has been marked as a duplicate of this bug. ***

[Don Olmstead]

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

[Ryosuke Niwa]

Created attachment 325253 [details]
Fixed the flaky tests

[WebKit Commit Bot]

Comment on attachment 325253 [details]
Fixed the flaky tests

Clearing flags on attachment: 325253

Committed r224146: <https://trac.webkit.org/changeset/224146>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

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.

[Lucas Forschler]

Mass moving XML DOM bugs to the "DOM" Component.