Bug 153683 – Layout Test fast/parser/external-entities-in-xslt.xml is flaky on El Capitan (but fails most of the time)

• Component: WebKit2
• Status: RESOLVED FIXED

[Ryan Haddad]

Layout Test fast/parser/external-entities-in-xslt.xml is flaky on El Capitan (but fails most of the time)

First failing run on El Capitan:
<https://build.webkit.org/builders/Apple%20El%20Capitan%20Release%20WK2%20(Tests)/builds/2905>

Flakiness Dashboard:
<http://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=fast%2Fparser%2Fexternal-entities-in-xslt.xml>

--- /Volumes/Data/slave/elcapitan-release-tests-wk2/build/layout-test-results/fast/parser/external-entities-in-xslt-expected.txt
+++ /Volumes/Data/slave/elcapitan-release-tests-wk2/build/layout-test-results/fast/parser/external-entities-in-xslt-actual.txt
@@ -1 +1,5 @@
+CONSOLE MESSAGE: line 1: Document is empty
+
+CONSOLE MESSAGE: line 1: Start tag expected, '<' not found
+
 SUCCESS

[Ryan Haddad]

Marked as flaky on El Capitan with <https://trac.webkit.org/r195864>

[Ryan Haddad]

Seems to have started with <http://trac.webkit.org/r195795>

[Alexey Proskuryakov]

This failure happens almost every time, so this has to be either this change or http://trac.webkit.org/changeset/195790 as the other potential culprit.

Other bots have been failing in the same way in the past, though.

[Chris Dumez]

This is printed by libxml2. I seem to remember investigating the same issue a while back where a test would cause these lines to get printed out on the following test case. I am trying to find that bug again.

[Alexey Proskuryakov]

Testing locally, this is pretty clearly <http://trac.webkit.org/r195795>. Dan, can you look into this?

What’s a good way to reproduce this locally with my build? Running that test by itself ("run-webkit-tests fast/parser/external-entities-in-xslt.xml") succeeds 10 out of 10 times.

^ with my Debug build, that is.

Also ran "run-webkit-tests fast/parser" successfully 4/4 times.

[Alexey Proskuryakov]

My guess is that it fails for you, but run-webkit-tests hides that, because the failure is expected. Running with "-v" will make the failure visible.

Good guess!

With an archived Debug build of r195795:

$ WebKitTestRunner LayoutTests/fast/parser/external-entities-in-xslt.xml 2> /dev/null
Content-Type: text/plain
CONSOLE MESSAGE: line 1: Document is empty

CONSOLE MESSAGE: line 1: Start tag expected, '<' not found

SUCCESS
#EOF
#EOF

$ DumpRenderTree LayoutTests/fast/parser/external-entities-in-xslt.xml 2> /dev/null
CONSOLE MESSAGE: line 1: Document is empty

CONSOLE MESSAGE: line 1: Start tag expected, '<' not found

Content-Type: text/plain
DumpMalloc: 55205888
SUCCESS
#EOF

--------------------------------------------------------------------------------
With an archived Debug build of r195794:

$ WebKitTestRunner LayoutTests/fast/parser/external-entities-in-xslt.xml 2> /dev/null
Content-Type: text/plain
CONSOLE MESSAGE: line 1: Document is empty

CONSOLE MESSAGE: line 1: Start tag expected, '<' not found

SUCCESS
#EOF
#EOF


$ DumpRenderTree LayoutTests/fast/parser/external-entities-in-xslt.xml 2> /dev/null
CONSOLE MESSAGE: line 1: Document is empty

CONSOLE MESSAGE: line 1: Start tag expected, '<' not found

Content-Type: text/plain
DumpMalloc: 54984704
SUCCESS
#EOF

--------------------------------------------------------------------------------

I don’t know why it’s OK for the ordering of the Content-Type: header with respect to the other output to differ between the tools, and I suspect that the ordering seen in WebKitTestRunner is related to the reported failure, but I don’t see evidence that that has changed recently.

It’s also worth noting that with both test tools, if I run the test a second time in the same instance of the tool, then the CONSOLE MESSAGE lines do not repeat. Consequently, if I use run-webkit-tests to run the test twice, it passes the second time.

Still, even though the WebKitTestRunner output looks the same (and wrong) to me before and after r195795, it looks different to run-webkit-tests, or run-webkit-tests gets it differently.

Created attachment 270474 [details]
Add missing keys to Web Content service environment

Created attachment 270475 [details]
Add missing keys to Web Content service environment

Thanks for the review! I can commit this fix, but I now think a better fix would be for the WebKitTestRunner injected bundle to set XML_CATALOG_NAME in its initialization. I guess I can do that in a follow-up.

[WebKit Commit Bot]

Comment on attachment 270475 [details]
Add missing keys to Web Content service environment

Clearing flags on attachment: 270475

Committed r196013: <http://trac.webkit.org/changeset/196013>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

Created attachment 270548 [details]
Better fix

Reopening to get a better fix reviewed.

[WebKit Commit Bot]

Comment on attachment 270548 [details]
Better fix

Clearing flags on attachment: 270548

Committed r196048: <http://trac.webkit.org/changeset/196048>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

Comment on attachment 270548 [details]
Better fix

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

> Tools/ChangeLog:11
> +          used by any code in the Web Content process. If that ever changed, we should send it over
> +          as a bundle parameter.

Just to have it here for posterity: we need to be careful with bundle parameters that affect which directories WebContent processes use, because UI process is not necessarily trusted.

Should be fine as long as the parameters are only respected for unsandboxed UI processes, such as WebKitTestRunner.

> Tools/Scripts/webkitpy/port/mac.py:-110
> -        # work around missing /etc/catalog <rdar://problem/4292995>
> -        env['XML_CATALOG_FILES'] = ''

Do we have the code to deal with this in DumpRenderTree?

> Tools/WebKitTestRunner/InjectedBundle/ios/InjectedBundleIOS.mm:39
> +    setenv("XML_CATALOG_FILES", "", 0);

Given that this test fails on other platforms too, perhaps this should be in cross-platform code.

(In reply to comment #21)
> Comment on attachment 270548 [details]
> Better fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=270548&action=review
> 
> > Tools/ChangeLog:11
> > +          used by any code in the Web Content process. If that ever changed, we should send it over
> > +          as a bundle parameter.
> 
> Just to have it here for posterity: we need to be careful with bundle
> parameters that affect which directories WebContent processes use, because
> UI process is not necessarily trusted.
> 
> Should be fine as long as the parameters are only respected for unsandboxed
> UI processes, such as WebKitTestRunner.

I wasn’t being clear. By “send it over as a bundle parameter” I meant writing code in WebKitTestRunner to use -[WKProcessPool _setObject:forBundleParameter:] or its C-SPI cousin to pass it along, not to change anything in WebKit itself.

> 
> > Tools/Scripts/webkitpy/port/mac.py:-110
> > -        # work around missing /etc/catalog <rdar://problem/4292995>
> > -        env['XML_CATALOG_FILES'] = ''
> 
> Do we have the code to deal with this in DumpRenderTree?

No, that’s why we still set the non-__XPC_-prefixed variable.

> 
> > Tools/WebKitTestRunner/InjectedBundle/ios/InjectedBundleIOS.mm:39
> > +    setenv("XML_CATALOG_FILES", "", 0);
> 
> Given that this test fails on other platforms too, perhaps this should be in
> cross-platform code.

I didn’t know that this problem affected other platforms. Has r195795 had any effect on non-Apple platforms, or have other platforms been failing forever and no one cared enough to fix them?

[Alexey Proskuryakov]

> No, that’s why we still set the non-__XPC_-prefixed variable.

Sorry, misread it - saw that the unprefixed line was red, and didn't look further.

> I didn’t know that this problem affected other platforms. Has r195795 had any effect on non-Apple platforms, or have other platforms been failing forever and no one cared enough to fix them?

The latter (I mentioned that in comment 3). And I guess that by other platforms, I only meant Windows and iOS, Linux ones seem ok:

<http://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=fast%2Fparser%2Fexternal-entities-in-xslt.xml>

I think that all this leaves us with two platforms to fix, Windows and iOS WK1 (because environment is not inherited from the proxy process to DumpRenderTree).

[Alexey Proskuryakov]

Oh, and still need to update TestExpectations, it seems?