| Summary: | Layout Test fast/parser/external-entities-in-xslt.xml is flaky on El Capitan (but fails most of the time) | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Ryan Haddad <ryanhaddad> | ||||||||
| Component: | WebKit2 | Assignee: | mitz | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | ap, cdumez, commit-queue, ddkilzer, glenn, koivisto, mitz | ||||||||
| Priority: | P2 | ||||||||||
| Version: | WebKit Nightly Build | ||||||||||
| Hardware: | Mac | ||||||||||
| OS: | OS X 10.11 | ||||||||||
| See Also: |
https://bugs.webkit.org/show_bug.cgi?id=17353 https://bugs.webkit.org/show_bug.cgi?id=143484 https://bugs.webkit.org/show_bug.cgi?id=143463 |
||||||||||
| Attachments: |
|
||||||||||
|
Description
Ryan Haddad
2016-01-29 16:05:22 PST
Marked as flaky on El Capitan with <https://trac.webkit.org/r195864> Seems to have started with <http://trac.webkit.org/r195795> 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. 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. 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. 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. 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> All reviewed patches have been landed. Closing bug. Created attachment 270548 [details]
Better fix
Reopening to get a better fix reviewed. Comment on attachment 270548 [details] Better fix Clearing flags on attachment: 270548 Committed r196048: <http://trac.webkit.org/changeset/196048> All reviewed patches have been landed. Closing bug. 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? > 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). Oh, and still need to update TestExpectations, it seems? |