1415217 - Make reftest error "load failed with unknown reason" more informative

Status: RESOLVED FIXED

(Testing :: Reftest, enhancement)

Description

[Jonathan Watt [:jwatt]]

The reftest error:

  REFTEST ERROR | load failed with unknown reason

isn't very helpful. We should add some diagnostics.

Comment 1

[Jonathan Watt [:jwatt]]

The series of events here is that reftest.jsm calls SendLoadTest/SendLoadScriptTest/SendLoadPrintTest resulting in RecvLoadTest/RecvLoadScriptTest/RecvLoadPrintTest being called in reftest-content.js. Those Recv* functions call StartTestURI, which calls:

  SetFailureTimeout(LoadFailed, timeout);

It's in LoadFailed that we call:

  SendFailedLoad(gFailureReason);

and it's when gFailureReason is null that we get the "load failed with unknown reason" message.

After a load is initiated when StartTestURI calls LoadURI, we continue handling the load in OnDocumentLoad (the event handler for "load" events). Once in OnDocumentLoad -- so long as we don't hit the early returns -- we will then go on to set gFailureReason in the if-else statement at the end of that function. Currently this is the point where gFailureReason is given a non-null value.

Comment 2

[Jonathan Watt [:jwatt]]

Attachment: patch

This changes the message so that it will read:

  timed out after <x> ms waiting for 'load' event for <uri>

That will allow checking that the timeout time is reasonable, that it looks consistent with the timestamps in the log file, and that the timeout is occurring for the URI that we're actually trying to load (not some other document due to us getting into some sort of bad state).

Comment 3

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 8926015 [details] [diff] [review]
patch

>Bug 1415217 - Make reftest error 'load failed with unknown reason' more informative. r=dholbert

This commit message isn't quite accurate -- please adjust.

Really, you're taking one scenario where we previously would output that message, and you're making us print something more specific.  But the old message ("load failed with unknown reason") still exists in our code and presumably can still be triggered in some cases (and you're not making those cases "more informative").


>diff --git a/layout/tools/reftest/reftest-content.js b/layout/tools/reftest/reftest-content.js
> 
>+  gFailureReason = "timed out after " + timeout +
>+                   " ms waiting for 'load' event for " + uri;
>   gFailureTimeout = setTimeout(wrapper, timeout);
> }

I need to take a closer look and convince myself that there aren't race conditions here (when I'm not in a CSSWG meeting) - hence, not r+'ing quite yet until I've been able to convince myself. Specifically, I want to watch out for cases where:
 - this gFailureReason might stick around even out after the timeout is canceled.
 - someone sets another gFailureReason before our timeout fires.

Hopefully those aren't possible (or to the extent that they're possible, they're intended/correct). :)

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 8926015 [details] [diff] [review]
patch

Review of attachment 8926015 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with commit message clarified as noted above, and with a code-commented added:

::: layout/tools/reftest/reftest-content.js
@@ +139,5 @@
>      }
>    }
>  
> +  gFailureReason = "timed out after " + timeout +
> +                   " ms waiting for 'load' event for " + uri;

Please add a code-comment here to clarify that this isn't necessarily the error-text that we're expecting to be logged -- it's only the error-text of last resort, kind of.  Maybe something like:

  // If we time out without making any progress, be sure we log something useful.
  // (If we make some progress before timing out and fail later on, we may
  // replace this with something more specific and log that instead.)

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/27a656fe5be8
Try to replace reftest error 'load failed with unknown reason' with informative messages. r=dholbert

Comment 7

[Jonathan Watt [:jwatt]]

Note that I also updated the "load failed with unknown reason" message in reftest.jsm to reflect the fact that we should never get this any more, along with the associated semi-obsolete comments surrounding it.

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/27a656fe5be8