|Summary:||REGRESSION(r143664, r143681): http/tests/security/feed-urls-from-remote.html fails|
|Product:||WebKit||Reporter:||Ryosuke Niwa <rniwa>|
|Component:||Tools / Tests||Assignee:||Eric Seidel (no email) <eric>|
|Severity:||Normal||CC:||abarth, ap, beidson, eric, japhet, webkit.review.bot, zan|
|Version:||528+ (Nightly build)|
Description Ryosuke Niwa 2013-02-21 21:55:26 PST
http://trac.webkit.org/changeset/143664 and http://trac.webkit.org/changeset/143681 made tests fail on Mac WK1 bots: http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40ToT%20-%20webkit.org&tests=http%2Ftests%2Fsecurity%2Ffeed-urls-from-remote.html e.g. http://build.webkit.org/results/Apple%20MountainLion%20Release%20WK1%20(Tests)/r143686%20(7135)/results.html --- /Volumes/Data/slave/mountainlion-release-tests-wk1/build/layout-test-results/http/tests/security/feed-urls-from-remote-expected.txt +++ /Volumes/Data/slave/mountainlion-release-tests-wk1/build/layout-test-results/http/tests/security/feed-urls-from-remote-actual.txt @@ -1,6 +1,7 @@ Policy delegate: attempt to load feed://127.0.0.1:8000/security/resources/feed.xml with navigation type 'link clicked' originating from HTML > #document +FAIL: Timed out waiting for notifyDone to be called < rdar://problem/5329440> REGRESSION: Clicking links with the feed:// protocol in Safari 3 does nothing This test is to see if a feed URL can be followed from a link on a page. -Test Finished. +Test not completed.
Comment 1 Ryosuke Niwa 2013-02-21 22:05:02 PST
Added a failing test expectation in http://trac.webkit.org/changeset/143689.
Comment 2 Eric Seidel (no email) 2013-02-21 22:32:36 PST
Yeah, we saw similar for Chromium, but I assumed it seemed specific to our DRT implementation. For now I'm just going to gate the offending code with ENABLE_THREADED_PARSER. That's slightly lame, and will potentially block enabling the threaded parser for other ports. This is a result of very very fragile code in the loader subsystem. The real solution is to re-write the loader, but that's out of scope for this change. :) It's also possible that we'll find a better solution than this one as we fix our last couple failures for the threaded parser. Thanks for filing.
Comment 3 Ryosuke Niwa 2013-02-21 22:34:36 PST
(In reply to comment #2) > > This is a result of very very fragile code in the loader subsystem. The real solution is to re-write the loader, but that's out of scope for this change. :) I don't think rewriting it really helps. The real problem with the loader is that we don't have a good strategy to test it.
Comment 6 Eric Seidel (no email) 2013-02-21 22:49:13 PST
(In reply to comment #3) > (In reply to comment #2) > > > > This is a result of very very fragile code in the loader subsystem. The real solution is to re-write the loader, but that's out of scope for this change. :) > > I don't think rewriting it really helps. The real problem with the loader is that we don't have a good strategy to test it. Perhaps. :) It's a state machine which is distributed between at least 5 classes, and about 20 bools. :) I suspect it would be a lot easier to understand as a single progressive state enum.
Comment 7 WebKit Review Bot 2013-02-21 23:20:52 PST
Comment on attachment 189688 [details] Patch Rejecting attachment 189688 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=gce-cq-01', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 189688, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue Last 500 characters of output: -conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' 54>At revision 13692. ________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' ________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' Updating webkit projects from gyp files... Updating OpenSource fatal: read error: Connection reset by peer Died at Tools/Scripts/update-webkit line 151. Full output: http://queues.webkit.org/results/16701504
Comment 8 Eric Seidel (no email) 2013-02-21 23:23:31 PST
Updating working directory Failed to run "['Tools/Scripts/update-webkit', '--chromium', '--force-update']" exit_code: 2 Updating OpenSource fatal: read error: Connection reset by peer Died at Tools/Scripts/update-webkit line 151. Odd.
Comment 9 WebKit Review Bot 2013-02-21 23:53:31 PST
Comment on attachment 189688 [details] Patch Clearing flags on attachment: 189688 Committed r143695: <http://trac.webkit.org/changeset/143695>
Comment 10 WebKit Review Bot 2013-02-21 23:53:35 PST
All reviewed patches have been landed. Closing bug.
Comment 11 Zan Dobersek 2013-02-22 05:36:21 PST
Committed r143719: <http://trac.webkit.org/changeset/143719>
Comment 12 Zan Dobersek 2013-02-22 05:43:18 PST
(In reply to comment #11) > Committed r143719: <http://trac.webkit.org/changeset/143719> Manually rolled this out in r143719 due to crashes occurring on debug builds of AppleMac WK1, EFL and GTK. Would have done the rollout through sheriffbot, but it was failing to update the checkout. Examples: http://build.webkit.org/results/Apple%20Lion%20Debug%20WK1%20(Tests)/r143709%20(6946)/results.html http://build.webkit.org/results/Apple%20MountainLion%20Debug%20WK1%20(Tests)/r143709%20(5750)/results.html http://build.webkit.org/results/EFL%20Linux%2064-bit%20Debug%20WK2/r143713%20(9747)/results.html http://build.webkit.org/results/GTK%20Linux%2064-bit%20Debug%20WK1/r143709%20(307)/results.html Given that this only seemed to fix one test and introduced crashes in multiple tests I reckoned rolling out is best.
Comment 13 Adam Barth 2013-02-22 08:50:19 PST
Yeah, Eric just missed the other half. Will fix shortly.
Comment 15 Eric Seidel (no email) 2013-02-22 10:27:47 PST
Sorry for the trouble.
Comment 16 Eric Seidel (no email) 2013-02-22 10:28:06 PST
Comment on attachment 189780 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=189780&action=review > Source/WebCore/dom/Document.cpp:5781 > + // FIXME: This should always be enabled, but it seems to cause > + // http/tests/security/feed-urls-from-remote.html to timeout on Mac WK1 > + // see http://webkit.org/b/110554 and http://webkit.org/b/110401 > +#if ENABLE(THREADED_HTML_PARSER) > loader()->checkLoadComplete(); > +#endif Didn't this part need a null check instead of an ENABLE?
Comment 17 Adam Barth 2013-02-22 10:35:47 PST
> Didn't this part need a null check instead of an ENABLE? That might of worked, but I wanted to keep isLoading and checkLoadComplete in sync.
Comment 18 WebKit Review Bot 2013-02-22 10:52:25 PST
Comment on attachment 189780 [details] Patch for landing Clearing flags on attachment: 189780 Committed r143753: <http://trac.webkit.org/changeset/143753>
Comment 19 WebKit Review Bot 2013-02-22 10:52:29 PST
All reviewed patches have been landed. Closing bug.