Closed Bug 1364596 Opened 3 years ago Closed Last year

the devtools DAMP test needs to be rewritten to be restartless and not use shims or addon SDK

Categories

(Testing :: Talos, enhancement)

Version 3
enhancement
Not set

Tracking

(firefox64 fixed)

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: rhelmer, Assigned: ochameau)

References

Details

Attachments

(1 file, 1 obsolete file)

The DAMP test added in bug 1150215 is non-restartless and uses shims and also the add-on SDK, all of which will be going away in Firefox 57 or soon thereafter.

Even if we were to make an exception for this add-on, using shims and add-on SDK in something measuring performance is likely to cause additional perf issues.
Brian, you are the original author for this - is this something you could help with?

This came up because we kmag had a backout due to test failure, and we're concerned that the DAMP test isn't going to work for much longer (57+). Also it is probably causing perf issues itself...
Flags: needinfo?(bgrinstead)
(In reply to Robert Helmer [:rhelmer] from comment #1)
> Brian, you are the original author for this - is this something you could
> help with?
> 
> This came up because we kmag had a backout due to test failure, and we're
> concerned that the DAMP test isn't going to work for much longer (57+). Also
> it is probably causing perf issues itself...

Looks like the only usages are:

  const { getActiveTab } = devtools.require("sdk/tabs/utils");
  const { getMostRecentBrowserWindow } = devtools.require("sdk/window/utils");

Which should be easy to replace with wm.getMostRecentWindow("navigator:browser").gBrowser.selectedTab.

However, devtools is in the process of converting to a system addon and moving the code out of m-c (currently aiming for June).  At that point it might not be possible to keep it running anyway.  Unless if there was a way to install a system addon prior to starting a talos test.. Joel, any idea about that?
Flags: needinfo?(bgrinstead) → needinfo?(jmaher)
I think the more important issue here is the e10s shims. The tests fail without shims enabled at least in part because they rely on being able to listen on "load" events from remote browsers. I wouldn't be surprised if there are other shims they rely on, too. Either way, just having the shims enabled makes performance test results questionable at best.
(In reply to Kris Maglione [:kmag] (busy; behind on reviews) from comment #3)
> I think the more important issue here is the e10s shims. The tests fail
> without shims enabled at least in part because they rely on being able to
> listen on "load" events from remote browsers. I wouldn't be surprised if
> there are other shims they rely on, too. Either way, just having the shims
> enabled makes performance test results questionable at best.

Is it just the browser.add/removeEventListener("load") calls that are a problem?  How can I test without shims enabled?
Pushed up a quick fix for sdk.  Works locally but try push just to be sure: https://treeherder.mozilla.org/#/jobs?repo=try&revision=145e7ecf050da33f2faba9d2d6146dd7227d438e
(In reply to Brian Grinstead [:bgrins] from comment #4)
> (In reply to Kris Maglione [:kmag] (busy; behind on reviews) from comment #3)
> > I think the more important issue here is the e10s shims. The tests fail
> > without shims enabled at least in part because they rely on being able to
> > listen on "load" events from remote browsers. I wouldn't be surprised if
> > there are other shims they rely on, too. Either way, just having the shims
> > enabled makes performance test results questionable at best.
> 
> Is it just the browser.add/removeEventListener("load") calls that are a
> problem?  How can I test without shims enabled?

As far as I know, yes. But I can't say for sure, since until those are fixed, I don't have any way to know if there are other issues.

You can test with shims disabled by adding `<em:multiprocessCompatible>true</em:multiprocessCompatible>` to the install.rdf file.
Talos tests have the ability to install any addons (currently .xpi or folder/) in the profile.  We are working on a webext test, so I imagine a system addon wouldn't be too difficult to get going.
Flags: needinfo?(jmaher)
Comment on attachment 8867403 [details]
Bug 1364596 - Remove SDK usage from the devtools DAMP test;

https://reviewboard.mozilla.org/r/138944/#review142474

we will need to sign this addon before deploying, I am happy to do that if you want as I need to sign the tabpaint addon- just let me know.
Attachment #8867403 - Flags: review?(jmaher) → review+
(In reply to Joel Maher ( :jmaher) from comment #9)
> Comment on attachment 8867403 [details]
> Bug 1364596 - Remove SDK usage from the devtools DAMP test;
> 
> https://reviewboard.mozilla.org/r/138944/#review142474
> 
> we will need to sign this addon before deploying, I am happy to do that if
> you want as I need to sign the tabpaint addon- just let me know.

That would be great but it sounds like there are still a few more changes needed for Comment 7
Depends on: 1388954
Comment on attachment 8867403 [details]
Bug 1364596 - Remove SDK usage from the devtools DAMP test;

This part has already landed in Bug 1388954
Attachment #8867403 - Attachment is obsolete: true
is there more work to do here?
Flags: needinfo?(poirot.alex)
1) It no longer uses SDK for a while now.

2) It seems to be a restartless add-on:
  https://searchfox.org/mozilla-central/source/testing/talos/talos/tests/devtools/addon/install.rdf#7
  <em:bootstrap>true</em:bootstrap>

  I think that's enough to say it is restartless, right?

3) Regarding comment 7, I think this was fixed by bug 1443964.
But I'll try to toggle `<em:multiprocessCompatible>true</em:multiprocessCompatible>` to verify that.


Otherwise, I heard about plans to deprecated anything that isn't WebExtension, but I imagine that should be investigated in a dedicated bug, if that's relevant.
Status: NEW → RESOLVED
Closed: Last year
Flags: needinfo?(poirot.alex)
Resolution: --- → FIXED
Oops, I didn't meant to resolve it quite yet.

Here is a try run with the multiprocess flag turned one:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d0177644e2d90ab044b8bc33886cceebadb2a36c
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
This is to know if DAMP works without e10s shims.

MozReview-Commit-ID: 2IZGlenkuzb
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/326840e3b097
Flag DAMP add-on as e10s compatible. r=jmaher
https://hg.mozilla.org/mozilla-central/rev/326840e3b097
Status: REOPENED → RESOLVED
Closed: Last yearLast year
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Assignee: nobody → poirot.alex
You need to log in before you can comment on or make changes to this bug.