[Windows] Intermittent Mochitest "test_access_control.html | Test timed out"

RESOLVED FIXED in mozilla1.9.2a1

Status

()

defect
RESOLVED FIXED
10 years ago
7 years ago

People

(Reporter: sgautherie, Assigned: cpearce)

Tracking

({fixed1.9.1, intermittent-failure})

Trunk
mozilla1.9.2a1
x86
Windows Server 2003
Points:
---
Dependency tree / graph
Bug Flags:
wanted1.9.1 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Reporter

Description

10 years ago
For example:
{
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1234700671.1234706254.29323.gz
Win2k3 comm-central dep unit test on 2009/02/15 04:24:31

[...]
result loaded*** 26918 INFO TEST-PASS | /tests/content/media/video/test/test_access_control.html | Can load from different port on same domain with allow-origin
NEXT ERROR *** 26919 ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/video/test/test_access_control.html | Test timed out.
}
and
{
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1234714851.1234720340.2182.gz
Win2k3 comm-central dep unit test on 2009/02/15 08:20:51
}

(And this failure then triggers bug 451949.)

This test was added by bug 451958:
could someone "Blocks" on it, and possibly cc me to it ?
Flags: wanted1.9.1?
Reporter

Updated

10 years ago
Blocks: 438871
Reporter

Updated

10 years ago
Blocks: 451958
Reporter

Comment 1

10 years ago
(In reply to comment #0)
> This test was added by bug 451958:
> could someone "Blocks" on it, and possibly cc me to it ?

Done :-)
Assignee

Comment 2

10 years ago
The patch for bug 476731 made some changes to the access controls test, to remove some intermittent failures that where showing up when working on that bug, so now that it's landed, does the test failure still show up?
Reporter

Comment 3

10 years ago
(In reply to comment #2)
> [...] bug 476731 [...] landed, does the test failure still show up?

Landed on trunk, but not on 1.9.1 yet ... so I can't tell wrt SeaMonkey tinderbox(es).

If you have examples of other tinderboxes failing (before/after), please add a comment.
Depends on: 476731
Reporter

Comment 4

10 years ago
(In reply to comment #3)
> Landed on trunk, but not on 1.9.1 yet

I was going to file a bug about the
{
result [...]*** 26905 INFO TEST-PASS | /tests/content/media/video/test/test_access_control.html | [...]
}
(wrong) output but I see this is already fixed on trunk :-)
Reporter

Comment 5

10 years ago
Firefox has a similar(/same !?) issue:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.1/1234885710.1234890838.16319.gz
WINNT 5.2 mozilla-1.9.1 unit test on 2009/02/17 07:48:30
{
result loaded*** 27260 INFO TEST-PASS | /tests/content/media/video/test/test_access_control.html | Can load from different port on same domain with allow-origin

command timed out: 300 seconds without output
program finished with exit code 1
}
Summary: [SeaMonkey, Windows] Intermittent "test_access_control.html | Test timed out" → [Windows] Intermittent Mochitest "test_access_control.html | Test timed out"
Flags: wanted1.9.1? → wanted1.9.1+
Whiteboard: [orange]
Today it timed out on Firefox trunk:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1236773232.1236777577.22588.gz#err0
OS X 10.5.2 mozilla-central unit test on 2009/03/11 05:07:12
Assignee

Comment 7

10 years ago
Ok, with these changes to test_access_control.html I can reproduce the bug hang.

This changes the testcase so that it loops infinitely trying an access-control-allowed and then disallowed load. After a while it will hang. It reproduces a lot more reliably in opt builds, and adding more logging tends to reduce the chance of the bug happening, so it's probably a timing issue.
Assignee

Comment 8

10 years ago
Ok, so the problem here is that the test is listening for loadeddata events, and sometimes we don't deliver them.

In a normal load, we usually receive these events:
loadstart progress durationchange loadedmetadata loadeddata progress canplay canplaythrough load

But sometimes we receive them in this order:
loadstart progress durationchange loadedmetadata canplay canplaythrough progress canplay canplaythrough load

The test is waiting on loadeddata which will never come, hence the failure.
Assignee

Comment 9

10 years ago
Posted patch Patch v1 (obsolete) — Splinter Review
* Rearrange nsHTMLMediaElement::ChangeReadyState() so that it always sends loaded data the first time readyState transitions to HAVE_CURRENT_DATA or greater.
Assignee: nobody → chris
Attachment #367713 - Flags: superreview?(roc)
Attachment #367713 - Flags: review?(chris.double)
(In reply to comment #9)
> Created an attachment (id=367713) [details]
> Patch v1
* Mochitests pass on Linux and Windows with this patch BTW.
+static const char* ReadyStateToString(nsMediaReadyState aState)
+{
+  switch (aState) {
+  case nsIDOMHTMLMediaElement::HAVE_NOTHING: return "HAVE_NOTHING";
+  case nsIDOMHTMLMediaElement::HAVE_METADATA: return "HAVE_METADATA";
+  case nsIDOMHTMLMediaElement::HAVE_CURRENT_DATA: return "HAVE_CURRENT_DATA";
+  case nsIDOMHTMLMediaElement::HAVE_FUTURE_DATA: return "HAVE_FUTURE_DATA";
+  case nsIDOMHTMLMediaElement::HAVE_ENOUGH_DATA: return "HAVE_ENOUGH_DATA";
+  default: return "ERROR_UNKNOWN_STATE";
+  }
+}

Just use a static const array and make it #ifdef DEBUG

+  if (mReadyState >= nsIDOMHTMLMediaElement::HAVE_CURRENT_DATA && 
+      oldState <= nsIDOMHTMLMediaElement::HAVE_METADATA &&
+      !mLoadedFirstFrame)

I think this would be more readable if we put the old state check first and wrot the same constant in both places:
  if (oldState < nsIDOMHTMLMediaElement::HAVE_CURRENT_DATA &&
      mReadyState >= nsIDOMHTMLMediaElement::HAVE_CURRENT_DATA &&
      !mLoadedFirstFrame)
Similar for the other oldState/mReadyState checks.

Hmm, can't we get rid of these nsIDOMHTMLMediaElement:: prefixes? Those constants should be inherited in here anyway.

I think we should break up the mReadyState == nsIDOMHTMLMediaElement::HAVE_ENOUGH_DATA actions into separate if statements. Then by changing the conditions we can share the code that triggers "canplay", and we can share the code that triggers "playing".
Posted patch Patch v2 (obsolete) — Splinter Review
> Hmm, can't we get rid of these nsIDOMHTMLMediaElement:: prefixes? Those
> constants should be inherited in here anyway.

Unfortunately not. nsHTMLMediaElement does not inherit from nsIDOMHTMLMediaElement, only nsHTMLVideoElement and nsHTMLAudioElement do.

I made the other changes you suggested. They allow a bit of code reuse, but I think the if-statement changes make it harder to see whether we follow the spec.

Tests still pass on Windows.
Attachment #367713 - Attachment is obsolete: true
Attachment #367722 - Flags: superreview?(roc)
Attachment #367722 - Flags: review?(chris.double)
Attachment #367713 - Flags: superreview?(roc)
Attachment #367713 - Flags: review?(chris.double)

Updated

10 years ago
Attachment #367722 - Flags: review?(chris.double) → review+
+  if (oldState < nsIDOMHTMLMediaElement::HAVE_FUTURE_DATA && 
+      (mReadyState == nsIDOMHTMLMediaElement::HAVE_FUTURE_DATA || 
+       mReadyState == nsIDOMHTMLMediaElement::HAVE_ENOUGH_DATA)) {

mReadyState >= nsIDOMHTMLMediaElement::HAVE_FUTURE_DATA

+     (mReadyState == nsIDOMHTMLMediaElement::HAVE_ENOUGH_DATA ||
+      mReadyState == nsIDOMHTMLMediaElement::HAVE_FUTURE_DATA) &&

Ditto

+      mReadyState == nsIDOMHTMLMediaElement::HAVE_ENOUGH_DATA) {

For symmetry, >= here

I like this code arrangement personally.
Attachment #367722 - Attachment is obsolete: true
Attachment #367940 - Flags: superreview?(roc)
Attachment #367940 - Flags: review+
Attachment #367722 - Flags: superreview?(roc)
Attachment #367940 - Flags: superreview?(roc) → superreview+
Assignee

Updated

10 years ago
Keywords: checkin-needed
Whiteboard: [orange] → [orange][needs landing]
Reporter

Comment 15

10 years ago
Comment on attachment 367940 [details] [diff] [review]
Patch v3
[Checkin: Comment 15 & 17]


http://hg.mozilla.org/mozilla-central/rev/aeada77bb251
Attachment #367940 - Attachment description: Patch v3 → Patch v3 [Checkin: Comment 15]
Reporter

Updated

10 years ago
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [orange][needs landing] → [orange]
Target Milestone: --- → mozilla1.9.2a1
Version: 1.9.1 Branch → Trunk
Reporter

Comment 16

10 years ago
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1237621406.1237628858.2843.gz
MacOSX 10.4 comm-central dep unit test on 2009/03/21 00:43:26
and triggered bug 481949:
looking forward to 1.9.1 landing.
Whiteboard: [orange] → [orange][needs 191 landing]
Reporter

Updated

10 years ago
Attachment #367940 - Attachment description: Patch v3 [Checkin: Comment 15] → Patch v3 [Checkin: Comment 15 & 17]
Whiteboard: [orange]
You need to log in before you can comment on or make changes to this bug.