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&)
<rdar://problem/35144665>
XSLT stuff should be moved out of the style code. It is there for historical reasons.
(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?
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
Created attachment 324877 [details] Cleanup
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 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?
(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 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.
(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.
Created attachment 324911 [details] Patch for landing
Comment on attachment 324911 [details] Patch for landing Wait for EWS again.
Committed r223999: <https://trac.webkit.org/changeset/223999>
Should we be worried at all about what happens before the timer fires? For example, what if something else removes the document from frame?
(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)
(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
Reverted r223999 for reason: Caused xsl LayoutTest flakiness. Committed r224116: <https://trac.webkit.org/changeset/224116>
*** Bug 178938 has been marked as a duplicate of this bug. ***
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
Created attachment 325253 [details] Fixed the flaky tests
Comment on attachment 325253 [details] Fixed the flaky tests Clearing flags on attachment: 325253 Committed r224146: <https://trac.webkit.org/changeset/224146>
All reviewed patches have been landed. Closing bug.
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.
Mass moving XML DOM bugs to the "DOM" Component.