Bug 60120 – editing/execCommand/append-node-under-document.html is flaky on chromium debug bots

• Component: New Bugs
• Status: RESOLVED WONTFIX

[Dirk Pranke]

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=editing%2FexecCommand%2Fappend-node-under-document.html&showExpectations=true&group=%40ToT%20-%20chromium.org

[Ryosuke Niwa]

Created attachment 112886 [details]
fix attempt

[WebKit Review Bot]

Comment on attachment 112886 [details]
fix attempt

Attachment 112886 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10230989

New failing tests:
editing/execCommand/append-node-under-document.html

[Ojan Vafai]

Comment on attachment 112886 [details]
fix attempt

Does this affect whether the test crashes? Or did the crash happen on line 15?

Also, why is document.open;document.writeln(...) flaky? Seems like there is a bug there.

[Ojan Vafai]

Comment on attachment 112886 [details]
fix attempt

I looked at the failing results. They just don't have the written out results. Isn't that a bug? i.e. isn't document.writeln synchronous?

[Ryosuke Niwa]

(In reply to comment #3)
> (From update of attachment 112886 [details])
> Does this affect whether the test crashes? Or did the crash happen on line 15?
> 
> Also, why is document.open;document.writeln(...) flaky? Seems like there is a bug there.

It appears that document.open has a similar effect to notifyDone. I can add waitUntilDone and notifyDone before/after those two statements but I'd rather not.

[Ojan Vafai]

(In reply to comment #5)
> (In reply to comment #3)
> > (From update of attachment 112886 [details] [details])
> > Does this affect whether the test crashes? Or did the crash happen on line 15?
> > 
> > Also, why is document.open;document.writeln(...) flaky? Seems like there is a bug there.
> 
> It appears that document.open has a similar effect to notifyDone. I can add waitUntilDone and notifyDone before/after those two statements but I'd rather not.

Except it's flaky. This only happens sometimes. Still seems like a bug. I don't see anything in this test that is timing dependent.

[Ojan Vafai]

Comment on attachment 112886 [details]
fix attempt

I would like to see an explanation of how this is timing dependent. If it always failed on all platforms, I would be OK with this change. But being flaky and on one platform implies a bug to me.

[Ryosuke Niwa]

Comment on attachment 112886 [details]
fix attempt

(In reply to comment #7)
> (From update of attachment 112886 [details])
> I would like to see an explanation of how this is timing dependent. If it always failed on all platforms, I would be OK with this change. But being flaky and on one platform implies a bug to me.

It was timing out on all debug builds, which seems to indicate that there's some race condition in loader, etc... But that's out of scope for this bug. I'd much rather have this test coverage by speculatively fix the test than having PASS FAIL expectation in test_expectations.txt because that's going to shadow any real failures we might have in the future.

[Ryosuke Niwa]

In fact, we'd go as far as to say we should add whatever work around or hack to make a test work unless the test is testing that particular loader race condition.

It's simply unacceptable for all these random race condition in loader/etc... to shadow real failures in editing.

[Ojan Vafai]

If we're going to change the test to no longer hit the bug, then the patch should come with a loader test that hits the same bug. It's not OK to just lose coverage.

[Ryosuke Niwa]

(In reply to comment #10)
> If we're going to change the test to no longer hit the bug, then the patch should come with a loader test that hits the same bug. It's not OK to just lose coverage.

I don't think it's a bug in loader either. document.open will replace the page and that's why DRT thinks the test has done running. Also, the test wasn't intended to test this loader/DRT behavior either. See http://trac.webkit.org/changeset/81185.

Each test should test precisely what it intends to test, no less, no more.

If you're objecting to this change, then I'm going to rollout r81185 and re-submit a new patch for the bug 56372 on the basis that the added test is flaky,

[Ojan Vafai]

(In reply to comment #11)
> I don't think it's a bug in loader either. document.open will replace the page and that's why DRT thinks the test has done running. Also, the test wasn't intended to test this loader/DRT behavior either. See http://trac.webkit.org/changeset/81185.

I still don't see what could cause this to be flaky that isn't a bug. It seems it should always pass or always fail. I don't understand the behavior of document.open well enough to know which it is.

> Each test should test precisely what it intends to test, no less, no more.

In general, I agree with you. At the same time, we shouldn't paper over bugs and pretend they are not there.

> If you're objecting to this change, then I'm going to rollout r81185 and re-submit a new patch for the bug 56372 on the basis that the added test is flaky,

I'm fine with changing this test to focus just on the editing bit. I just would like to see this patch come along with a test that exposes the loader bug as well, with a bug filed to track getting it fixed.

In theory, you can just copy-paste your current test and remove the editing bits, no?

If we always paper over the flakiness, then the tests and possibly the shipping product will always be flaky.

[Ryosuke Niwa]

(In reply to comment #12)
> (In reply to comment #11)
> > I don't think it's a bug in loader either. document.open will replace the page and that's why DRT thinks the test has done running. Also, the test wasn't intended to test this loader/DRT behavior either. See http://trac.webkit.org/changeset/81185.
> 
> I still don't see what could cause this to be flaky that isn't a bug. It seems it should always pass or always fail. I don't understand the behavior of document.open well enough to know which it is.

Calling document.open() is the problem. As far as I understand it, calling that then calling document.writeln results in an invocation of dispatchDecidePolicyForNavigationAction > decidePolicyForNavigation > policyDelegateDone > testFinished.

> > Each test should test precisely what it intends to test, no less, no more.
> 
> In general, I agree with you. At the same time, we shouldn't paper over bugs and pretend they are not there.

I don't think this is a bug. DRT and WebKit are behaving as expected as far as I understand it.

[Stephen Chenney]

LayoutTest failures for Chromium are being marked WontFix. The Bug is still accessible and referenced from TestExpectations.