regression from talos webserver- talos damp doesn't load a real page

RESOLVED FIXED in Firefox 45, Firefox OS v2.5

Status

Testing
Talos
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jmaher, Assigned: jmaher)

Tracking

unspecified
mozilla45
Points:
---

Firefox Tracking Flags

(firefox45 fixed, b2g-v2.5 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

2 years ago
in bug 1195288, we switched to using a python webserver, this had an unintended consequence where we don't point to a source page for damp anymore:
https://dxr.mozilla.org/mozilla-central/source/testing/talos/talos/tests/devtools/addon/content/damp.js?case=true&from=damp.js#9

this is fairly easy to fix, we pass in a webserver for tps, we can do a similar thing here.
(Assignee)

Comment 1

2 years ago
Created attachment 8669097 [details] [diff] [review]
damp_webserver.patch

I still need to test this out a bit more, but this should solve the problem.  Apologies for letting this in!
Assignee: nobody → jmaher
Status: NEW → ASSIGNED
Attachment #8669097 - Flags: review?(bgrinstead)
Comment on attachment 8669097 [details] [diff] [review]
damp_webserver.patch

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

Matches what I see for addon.test.tabswitch.webserver, so LGTM!  How will this affect being able to `./mach talos-test -a damp` locally?  Is there a way to specify that web server in the command?
Attachment #8669097 - Flags: review?(bgrinstead) → review+
(Assignee)

Comment 3

2 years ago
right now ./mach needs a bit of updating since we moved in-tree, I tested in tree like this:
cd testing/talos
virtualenv .
. bin/activate
cd talos
python run_tests.py -e ../../../objdir/dist/bin/firefox -a damp --develop

--develop is the key you are looking for, mach does this automatically- with the change which landed earlier this week for all talos to use the python webserver, --develop only dumps the output to local files.

Comment 4

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/cbd675d08907
(In reply to Joel Maher (:jmaher) from comment #3)
> --develop is the key you are looking for, mach does this automatically- with
> the change which landed earlier this week for all talos to use the python
> webserver, --develop only dumps the output to local files.

So --develop and then where do you put the tp5n page set?
(Assignee)

Comment 6

2 years ago
sorry, the magic detail:

cd testing/talos/talos/tests
wget <path>/tp5n.zip
unzip tp5n.zip

that is all the magic :)
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/b3da7bfd13b6 for WinXP g2 permabustage like https://treeherder.mozilla.org/logviewer.html#?job_id=15124487&repo=mozilla-inbound
Hmm if that's right that mean that this patch makes the browser hang. (?)

The error then comes from mozcrash, I already reported that in bug 1208823 - there is an *issue* to extract minidump on windows. Though if you look at the log dates, there is a jump from "13:56:02" to "14:53:22", and talos try to kill the hanging browser.
See Also: → bug 1208823
(Assignee)

Comment 9

2 years ago
well, even if mozcrash is fixed, it means that damp will be crashing and not running cleanly on xp.  DAMP doesn't create many tabs, it just deals with a couple at a time.  I wonder if we have memory issues with the devtools which now that we have a python webserver it is causing issues.

We know that a 404 page works, maybe a simpler page would be successful as bild.de might be a heavier webpage.  That would at least give credibility to the memory issue.  All of the other tests which use the pageset do not have issues on bild.de.
(In reply to Joel Maher (:jmaher) from comment #9)
> well, even if mozcrash is fixed, it means that damp will be crashing and not
> running cleanly on xp.  DAMP doesn't create many tabs, it just deals with a
> couple at a time.  I wonder if we have memory issues with the devtools which
> now that we have a python webserver it is causing issues.
> 
> We know that a 404 page works, maybe a simpler page would be successful as
> bild.de might be a heavier webpage.  That would at least give credibility to
> the memory issue.  All of the other tests which use the pageset do not have
> issues on bild.de.

We could test that by doing a try build with your patch + another one that switches COMPLICATED_URL to some other page in the tp5n set: https://dxr.mozilla.org/mozilla-central/source/testing/talos/talos/tests/devtools/addon/content/damp.js#9
(Assignee)

Comment 11

2 years ago
this is fairly easy to reproduce on a loaner windows xp machine and I get an error in the error console of firefox:
error getting sources: undefined
http://mxr.mozilla.org/mozilla-central/source/devtools/client/debugger/debugger-controller.js#1296


this is easy to reproduce, it shows up while loading/refreshing the bild.de page (we do many prior, this just shows up a minute or so into the test) while the debugger window is open.  running on a different page such as youtube.com doesn't get us there.  


We just need to figure out how to find out aresoponse.message is undefined.
(Assignee)

Comment 12

2 years ago
adding some simple dump statements to determine where we are hanging, I found:
reloadpage, complicated.styleeditor

after hacking around with some direction by bgrins, nothing obvious is showing up- it looks as though a reload of the page does help us move forward (i.e. we are not getting the load event).  A few trials yielded this to not always be successful, maybe it is a timing thing where reloading right when it hangs doesn't help, but after a bit of time it does move on.
And maybe most oddly, the hang still happens even if we make openToolbox and closeToolbox do nothing so that the test is reduced to adding a tab and reloading it.  Something about the bild.de page seems to be causing a situation where the 'load' event is not firing on the browser after causing browser.reload()
(Assignee)

Comment 14

2 years ago
Created attachment 8683280 [details] [diff] [review]
use a real webpage for damp, disable on winxp (2.0)

tested on try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4eaae7f9dc84
Attachment #8669097 - Attachment is obsolete: true
Attachment #8683280 - Flags: review?(bgrinstead)
Comment on attachment 8683280 [details] [diff] [review]
use a real webpage for damp, disable on winxp (2.0)

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

::: testing/talos/talos/config.py
@@ +305,5 @@
>          raise ConfigurationError("No definition found for test(s): %s"
>                                   % missing)
> +
> +    # disabled DAMP on winXP: frequent hangs, <3% of devtools users on winXP
> +    if utils.PLATFORM_TYPE == 'win_':

How does this limit skipping to only winXP?  Seems fine on the try push, but don't know anything about this PLATFORM_TYPE thing

::: testing/talos/talos/tests/devtools/addon/content/damp.js
@@ +6,5 @@
>  const { getActiveTab } = devtools.require("sdk/tabs/utils");
>  const { getMostRecentBrowserWindow } = devtools.require("sdk/window/utils");
>  const ThreadSafeChromeUtils = devtools.require("ThreadSafeChromeUtils");
>  
> +webserver = Services.prefs.getCharPref("addon.test.damp.webserver");

`const webserver` (unless if this is declared somewhere else)
Attachment #8683280 - Flags: review?(bgrinstead) → review+
(Assignee)

Comment 16

2 years ago
yes, the magical "utils.PLATFORM_TYPE == 'win_'" will differentiate for windows xp;  I did outline that with a comment, possibly we could find a more comment method for doing this across talos- if you would like I could file a bug for that.

I will add a const to delcare the webserver, thanks for catching that.
(In reply to Joel Maher (:jmaher) from comment #16)
> yes, the magical "utils.PLATFORM_TYPE == 'win_'" will differentiate for
> windows xp;  I did outline that with a comment, possibly we could find a
> more comment method for doing this across talos- if you would like I could
> file a bug for that.

No big deal - I saw the comment and figured it did the right thing, just haven't seen that code before

Comment 18

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/edc82c25a1f9

Comment 19

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/edc82c25a1f9
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45

Comment 20

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/84030b7af73e
status-b2g-v2.5: --- → fixed
You need to log in before you can comment on or make changes to this bug.