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

RESOLVED FIXED in Firefox 58

Status

Testing
Reftest
RESOLVED FIXED
17 days ago
15 days ago

People

(Reporter: jwatt, Assigned: jwatt)

Tracking

Trunk
mozilla58
Points:
---

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

The reftest error:

  REFTEST ERROR | load failed with unknown reason

isn't very helpful. We should add some diagnostics.
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.
(Assignee)

Updated

17 days ago
Summary: Make reftest error "load failed with unknown reason" less likely → Make reftest error "load failed with unknown reason" more informative
Created attachment 8926013 [details] [diff] [review]
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).
Attachment #8926013 - Flags: review?(dholbert)
Created attachment 8926015 [details] [diff] [review]
patch
Attachment #8926013 - Attachment is obsolete: true
Attachment #8926013 - Flags: review?(dholbert)
Attachment #8926015 - Flags: review?(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 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.)
Attachment #8926015 - Flags: review?(dholbert) → review+

Comment 6

16 days ago
Pushed by jwatt@jwatt.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/27a656fe5be8
Try to replace reftest error 'load failed with unknown reason' with informative messages. r=dholbert
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.
https://hg.mozilla.org/mozilla-central/rev/27a656fe5be8
Status: NEW → RESOLVED
Last Resolved: 15 days ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.