Bug 110554 – REGRESSION(r143664, r143681): http/tests/security/feed-urls-from-remote.html fails

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Added a failing test expectation in http://trac.webkit.org/changeset/143689.

[Eric Seidel (no email)]

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.

[Ryosuke Niwa]

(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.

[Eric Seidel (no email)]

Created attachment 189688 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 189688 [details]
Patch

rs=me.

[Eric Seidel (no email)]

(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.

[WebKit Review Bot]

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

[Eric Seidel (no email)]

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.

[WebKit Review Bot]

Comment on attachment 189688 [details]
Patch

Clearing flags on attachment: 189688

Committed r143695: <http://trac.webkit.org/changeset/143695>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Zan Dobersek]

Committed r143719: <http://trac.webkit.org/changeset/143719>

[Zan Dobersek]

(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.

[Adam Barth]

Yeah, Eric just missed the other half.  Will fix shortly.

[Adam Barth]

Created attachment 189780 [details]
Patch for landing

[Eric Seidel (no email)]

Sorry for the trouble.

[Eric Seidel (no email)]

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?

[Adam Barth]

> 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.

[WebKit Review Bot]

Comment on attachment 189780 [details]
Patch for landing

Clearing flags on attachment: 189780

Committed r143753: <http://trac.webkit.org/changeset/143753>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.