Bug 110554 - REGRESSION(r143664, r143681): http/tests/security/feed-urls-from-remote.html fails
Summary: REGRESSION(r143664, r143681): http/tests/security/feed-urls-from-remote.html ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eric Seidel (no email)
URL:
Keywords: LayoutTestFailure
Depends on:
Blocks:
 
Reported: 2013-02-21 21:55 PST by Ryosuke Niwa
Modified: 2013-02-22 10:52 PST (History)
7 users (show)

See Also:


Attachments
Patch (1.69 KB, patch)
2013-02-21 22:36 PST, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Patch for landing (2.16 KB, patch)
2013-02-22 09:14 PST, Adam Barth
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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 4 Eric Seidel (no email) 2013-02-21 22:36:44 PST
Created attachment 189688 [details]
Patch
Comment 5 Ryosuke Niwa 2013-02-21 22:39:15 PST
Comment on attachment 189688 [details]
Patch

rs=me.
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 14 Adam Barth 2013-02-22 09:14:07 PST
Created attachment 189780 [details]
Patch for landing
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.