Bug 153683 - Layout Test fast/parser/external-entities-in-xslt.xml is flaky on El Capitan (but fails most of the time)
Summary: Layout Test fast/parser/external-entities-in-xslt.xml is flaky on El Capitan ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Macintosh OS X 10.11
: P2 Normal
Assignee: mitz
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-01-29 16:05 PST by Ryan Haddad
Modified: 2016-02-02 23:36 PST (History)
7 users (show)

See Also:


Attachments
Add missing keys to Web Content service environment (3.65 KB, text/plain)
2016-02-01 23:36 PST, mitz
no flags Details
Add missing keys to Web Content service environment (4.95 KB, patch)
2016-02-01 23:37 PST, mitz
no flags Details | Formatted Diff | Diff
Better fix (5.61 KB, patch)
2016-02-02 21:35 PST, mitz
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryan Haddad 2016-01-29 16:05:22 PST
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
Comment 1 Ryan Haddad 2016-01-29 16:35:38 PST
Marked as flaky on El Capitan with <https://trac.webkit.org/r195864>
Comment 2 Ryan Haddad 2016-01-29 16:36:45 PST
Seems to have started with <http://trac.webkit.org/r195795>
Comment 3 Alexey Proskuryakov 2016-01-30 17:06:04 PST
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.
Comment 4 Chris Dumez 2016-02-01 09:35:31 PST
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.
Comment 5 Alexey Proskuryakov 2016-02-01 18:06:38 PST
Testing locally, this is pretty clearly <http://trac.webkit.org/r195795>. Dan, can you look into this?
Comment 6 mitz 2016-02-01 18:09:41 PST
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.
Comment 7 mitz 2016-02-01 18:10:11 PST
^ with my Debug build, that is.
Comment 8 mitz 2016-02-01 18:45:27 PST
Also ran "run-webkit-tests fast/parser" successfully 4/4 times.
Comment 9 Alexey Proskuryakov 2016-02-01 22:00:59 PST
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.
Comment 10 mitz 2016-02-01 22:03:07 PST
Good guess!
Comment 11 mitz 2016-02-01 22:54:27 PST
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.
Comment 12 mitz 2016-02-01 23:36:29 PST
Created attachment 270474 [details]
Add missing keys to Web Content service environment
Comment 13 mitz 2016-02-01 23:37:50 PST
Created attachment 270475 [details]
Add missing keys to Web Content service environment
Comment 14 mitz 2016-02-02 09:19:37 PST
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.
Comment 15 WebKit Commit Bot 2016-02-02 10:06:55 PST
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>
Comment 16 WebKit Commit Bot 2016-02-02 10:07:00 PST
All reviewed patches have been landed.  Closing bug.
Comment 17 mitz 2016-02-02 21:35:51 PST
Created attachment 270548 [details]
Better fix
Comment 18 mitz 2016-02-02 21:36:07 PST
Reopening to get a better fix reviewed.
Comment 19 WebKit Commit Bot 2016-02-02 22:14:17 PST
Comment on attachment 270548 [details]
Better fix

Clearing flags on attachment: 270548

Committed r196048: <http://trac.webkit.org/changeset/196048>
Comment 20 WebKit Commit Bot 2016-02-02 22:14:21 PST
All reviewed patches have been landed.  Closing bug.
Comment 21 Alexey Proskuryakov 2016-02-02 23:17:21 PST
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.
Comment 22 mitz 2016-02-02 23:23:04 PST
(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?
Comment 23 Alexey Proskuryakov 2016-02-02 23:35:14 PST
> 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).
Comment 24 Alexey Proskuryakov 2016-02-02 23:36:21 PST
Oh, and still need to update TestExpectations, it seems?