580306 - Sisyphus - Crash Automation - Spider fires onload prematurely.

Status: RESOLVED FIXED

(Testing Graveyard :: Sisyphus, defect)

Description

[Bob Clary [:bc] (inactive)]

Attachment: patch v1

Mohamed found a bug in Spider where the onload handler would fire at the first load event it received. This is incorrect due to the xul:browser element, used to contain loaded pages, receiving load events from the images, iframes, etc loaded by the page prior to the page being completely loaded.

The consequence of this bug is the user hook runs before the page has completely loaded.

This patch keeps track of the originally requested page (and any possible redirections from that page) to ignore load events that are not associated with the page load event we are interested in.

Comment 1

[cmtalbert]

Comment on attachment 458710 [details] [diff] [review]
patch v1

>From: Bob Clary <[e-mail]>
>
>diff --git a/spider/chrome/content/spider/spider.js b/spider/chrome/content/spider/spider.js
>     try
>     {
>       response.originalURI = httpChannel.originalURI.spec;
>     }
>     catch(ex)
>     {
>-      window.dlog(ex);
>+      window.dlog('gHTTPResponseObserver.observe: ' + ex);
>     }
> 
>     try
>     {
>       response.URI = httpChannel.URI.spec;
>     }
>     catch(ex)
>     {
>-      window.dlog(ex);
>+      window.dlog('gHTTPResponseObserver.observe: ' + ex);
>     }
> 
>     try
>     {
>       response.referrer = httpChannel.referrer.spec;
>     }
>     catch(ex)
>     {
>-      window.dlog(ex);
>+      window.dlog('gHTTPResponseObserver.observe: ' + ex);
>     }
> 
>     try
>     {
>       response.responseStatus = httpChannel.responseStatus;
>     }
>     catch(ex)
>     {
>-      window.dlog(ex);
>+      window.dlog('gHTTPResponseObserver.observe: ' + ex);
>     }
> 
>     try
>     {
>       response.responseStatusText = httpChannel.responseStatusText.
>       toLowerCase();
>     }
>     catch(ex)
>     {
>-      window.dlog(ex);
>+      window.dlog('gHTTPResponseObserver.observe: ' + ex);
>     }
> 
>     try
>     {
>       response.requestSucceeded = httpChannel.requestSucceeded;
>     }
>     catch(ex)
>     {
>-      window.dlog(ex);
>+      window.dlog('gHTTPResponseObserver.observe: ' + ex);
>     }
> 
>     try
>     {
>       response.contentType = httpChannel.getResponseHeader('content-type');
>     }
>     catch(ex)
>     {
>-      window.dlog(ex);
>+      window.dlog('gHTTPResponseObserver.observe: ' + ex);
>     }
>+
>+    try
>+    {
>+      response.name = httpChannel.name;
>+    }
>+    catch(ex)
>+    {
>+      window.dlog('gHTTPResponseObserver.observe: ' + ex);
>+    }
>+
>+    response.location = '';
>+    try {
>+      response.location = httpChannel.getResponseHeader('location');
>+    }
>+    catch(ex) {
>+      window.dlog('gHTTPResponseObserver.observe: ' + ex);
>+    }
I'm not sure how these errors are bubbled up, but do you really want the exact same error message thrown regardless of which item failed?  It seems like it'd be much easier to debug if each of these try/catch's said something like what they were doing when they caught an error: 'gHTTPResponseObserver.observe::Could not get response header: ' + ex.  It seems odd that you went through all the trouble to wrap them all in try/catches but not to call out why each one is failing.

Does the exception generally give you enough detail here so more text is unneeded?

It's up to you if you want to change that.  This looks like a good approach, so r=ctalbert.

Comment 2

[Bob Clary [:bc] (inactive)]

I'll tweak the debug message before committing. Thanks.

Comment 3

[Bob Clary [:bc] (inactive)]

http://hg.mozilla.org/automation/sisyphus/rev/9fd718d254fc with wrong bugnumber damnit.

Comment 4

[Bob Clary [:bc] (inactive)]

This didn't quite fix everything.

Comment 5

[Bob Clary [:bc] (inactive)]

Attachment: patch 2 v1

This patch:

* handles redirections involving relative paths.

* adds http response 303 to the responses treated as redirections.

* adds http-on-example-cached-response to the http responses being observed.

* removes onbeforeunload handlers which cause the spider to get stuck on pages.

* checks DOMContentLoaded events in addition to load events.

* adds an additional load handler to the content window when the DOMContentLoaded event matches the expected url as a fall back.

* adds a 60 second time out after DOMContentLoaded to force the load to be handled.

hitch hikers:

added new Window button to open a browser window from Spider.

This patch has lots of whitespace changes. I'll attach a hg qdiff -wbB -U8 patch for actual inspection.

Comment 6

[Bob Clary [:bc] (inactive)]

Attachment: patch 2 v1 (no whitespace)

Comment 7

[cmtalbert]

Comment on attachment 471218 [details]
patch 2 v1

I reviewed the patch without whitespace changes and it looked good.

Comment 8

[Bob Clary [:bc] (inactive)]

http://hg.mozilla.org/automation/sisyphus/rev/e85336b8caaa