715588 - talos has enablePrivilege deprecated warning, need to use specialpowers

Status: RESOLVED WONTFIX

(Testing :: Talos, defect)

Description

[Joel Maher ( :jmaher ) (UTC -8)]

there is a warning printed in the ts logs about enablePrivilege being deprecated.  We can use a specialpowers replacement and should.  

This means that pageloader, fennecmark, e10s and all talos proper code needs to be updated as well.

Comment 1

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: add specialpowers to talos, remove other e10s related code (0.9)

There are a lot of corner cases in Talos as well as awkward mobile constraints.  This patch is starting to show up green on try server, if it is green tomorrow morning, I will clean this up and ask for a review.

Comment 2

[Bobby Holley (:bholley)]

Hm, seems unfortunate to have two copies of SpecialPowers. I guess the one in mochitests will be canonical, and we can sync it over periodically? We should definitely forbid patching the Talos one directly, lest they drift out of sync.

Comment 3

[Joel Maher ( :jmaher ) (UTC -8)]

we copy a lot of things into the talos tree, this will have to be one of them.  I have also added a few new things into special powers that are unique to talos.  We can work on getting those into the mochitest specialpowers and making this an almost identical copy.  The problem is in mochitest specialpowers there is a mockfilepicker and that is not needed or in my local copy of specialpowers.  

This is looking really good except for android-xul tests which will require me to land changes and get the android webserver udpated before I can test these changes on try server.  I did verify that exact build and commands (sans networking related options) work on my local tegra.

Comment 4

[Joel Maher ( :jmaher ) (UTC -8)]

I just realized a flaw in this, all branches use bm-remote, so we would need to deploy to all branches.

Comment 5

[Joel Maher ( :jmaher ) (UTC -8)]

or leave a lot of the enablePrivilege code in place and figure out a way to try/except around it so we don't have to resolve this on all branches at once.

:bholley, if we had an enablePrivilege call inside a try/catch, what would happen in the IonMonkey scenario?

Comment 6

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: add specialpowers to talos, remove other e10s related code (1.0)

cleaned up patch, runs clean.  There is only the issue of landing this and maybe leaving stuff in to support branches where we don't have this installed (1.9.2, esr, release, beta, aurora).  I would like to get the feedback for this now so we can at least resolve the majority of this.

Comment 7

[Bobby Holley (:bholley)]

(In reply to Joel Maher (:jmaher) from comment #5)
> :bholley, if we had an enablePrivilege call inside a try/catch, what would
> happen in the IonMonkey scenario?

The call would succeed, unfortunately. So the issue is that we need to use the same talos code for everything? Could we sniff the UA string or something?

Comment 8

[Joel Maher ( :jmaher ) (UTC -8)]

yeah, the older branches which won't have an updated talos.  We could land talos on those branches, but we need to do try server runs and gain approval for each branch.  

The UA string should work.  Is there anything javascript specific that we could test for?

Comment 9

[Bobby Holley (:bholley)]

(In reply to Joel Maher (:jmaher) from comment #8)
> The UA string should work.  Is there anything javascript specific that we
> could test for?

bent, what's the best way to sniff the browser version from JS? navigator.something?

Comment 10

[Joel Maher ( :jmaher ) (UTC -8)]

This turns into a chicken and egg problem.  As soon as we update bm-remote, all talos tests will fail unless we have a patch checked into m-c and everywhere else.  I propose this:
* get specialpowers aware code landed on m-c, let it land on m-i and other branches as well
* determine a method for triggering runs without specialpowers
* change bm-remote to have conditional ua string
** now talos will be able to use specialpowers instead of enablePrivilege
* file a bug to remove enablePrivilege when talos has been updated everywhere
** consider getting aurora approval to speed this process up.

Comment 11

[Jeff Hammel]

Comment on attachment 632313 [details] [diff] [review]
add specialpowers to talos, remove other e10s related code (1.0)

I didn't read the specialpowers files, as I assume they are direct ports from m-c. Assuming so, this looks good to me.

Comment 12

[Joel Maher ( :jmaher ) (UTC -8)]

I am really stuck on how to get this to work with enablePrivilege for older branches and specialpowers for the newer branch.  It really boils down to having javascript files that I need to include in my test pages conditionally.  Maybe my feeble attempt was a misuse of technology and somebody who knows more about javascript and how the scoping and interpreting of it could help me figure this out.

Comment 13

[Bobby Holley (:bholley)]

(In reply to Joel Maher (:jmaher) from comment #12)
> I am really stuck on how to get this to work with enablePrivilege for older
> branches and specialpowers for the newer branch.  It really boils down to
> having javascript files that I need to include in my test pages
> conditionally.  Maybe my feeble attempt was a misuse of technology and
> somebody who knows more about javascript and how the scoping and
> interpreting of it could help me figure this out.

I'm not sure I understand the issue here. Why do you need to conditionally include things? Why not just have two functions and call whichever one is appropriate? A function that calls enablePrivilege doesn't hurt anything as long as the function is never called.

Comment 14

[Joel Maher ( :jmaher ) (UTC -8)]

I was planning to define log and quit functions conditionally so when we had the ability to remove enablePrivilege for good we could remove a couple small bits of code and not have to retrofit everything.

It is a little more complicated than just defining some functions as we do some stuff by default (http://hg.mozilla.org/build/talos/file/c6aba61c0f78/scripts/MozillaFileLogger.js#l159), but I can work around that.

Comment 15

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: add specialpowers to talos (2.0)

this is updated and it adds a ua string check to two files.  Also I moved the logic into our two files MozillaFileLogger.js and quit.js.  This way we can call the same API in the rest of the files.  

One problem I encountered was a namespace collision, so I renamed existing MozillaFileLogger objects to MozFileLogger.  

Regarding specialpowers, I added a handful of functionality to it and removed stuff we do not use.  So this is different than that of mochitest, but I don't see us needing to adjust this much.  I would prefer to keep this extension smaller as this is for performance tests and not just conformance unit tests.

Comment 16

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 633572 [details] [diff] [review]
add specialpowers to talos (2.0)

jhammel: can you review the talos parts of this
bholley: can you review the ua parsing and the specialpowers

Comment 17

[Bobby Holley (:bholley)]

Comment on attachment 633572 [details] [diff] [review]
add specialpowers to talos (2.0)

Some comments and examples about the user-agent strings would be nice. Also, can we print some sort of tinderbox-visible log of whether we are or are not using SpecialPowers?

This all looks reasonable, but the talos setup is totally foreign to me. Maybe ted is a better reviewer?

Comment 18

[Jeff Hammel]

Comment on attachment 633572 [details] [diff] [review]
add specialpowers to talos (2.0)

+    const FIREFOX_ID = '{ec8030f7-c20a-464f-9b0e-13a3a9e97384}';
+    const THUNDERBIRD_ID = '{3550f703-e582-4d05-9a08-453d09bdfdc6}';
+    const SEAMONKEY_ID = '{92650c4d-4b8e-4d2a-b7eb-24ecf4f6b63a}';
+    const FENNEC_ID = '{aa3c5121-dab2-40e2-81ca-7ea25febc110}';
+    const XUL_FENNEC_ID = '{a23983c0-fd0e-11dc-95ff-0800200c9a66}';
+    product = 'undefined';
+
+    if(id == FIREFOX_ID) {  
+      product = 'Firefox';
+    } else if(id == THUNDERBIRD_ID) {  
+      product = 'Thunderbird';
+    } else if(id == SEAMONKEY_ID) {
+      product = 'SeaMonkey';
+    } else if(id == XUL_FENNEC_ID) {
+      product = 'Fennec';
+    } else if(id == FENNEC_ID) {
+      product = 'Fennec';
     }

Nit: I'd prefer these in a hash mapping versus an if-else tree.

+// Detect if we are on older branches that don't have specialpowers enabled talos available
+var ua_plat = window.navigator.userAgent.split('(')[1].split(')')[0];

Pretty magical but I guess there is little alternative :(  It would be nice not to repeat this code.  Is that possible?

Looks good!  Assuming its test, r=me

Comment 19

[Joel Maher ( :jmaher ) (UTC -8)]

pushed, need to update bm-remote and test on try.  I can handle any fallouts in a followup patch asap as needed.

Comment 20

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 633572 [details] [diff] [review]
add specialpowers to talos (2.0)

decided we had enough input on this to land and go forward.

Comment 21

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: updated to enableprivilege patch for talos (1.0)

updated to fix a couple small things with enableprivilege and allow for testing with --develop on try server!

Comment 22

[Jeff Hammel]

Comment on attachment 637096 [details] [diff] [review]
updated to enableprivilege patch for talos (1.0)

+    if tree != None and tree != '':

Why not just `if tree:` ?

lgtm

Comment 23

[Joel Maher ( :jmaher ) (UTC -8)]

landed on talos with 'if tree':
http://hg.mozilla.org/build/talos/rev/18898c970f8a

Next steps are to update the bm-remote and then to land talos.

Comment 24

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: add check if specialpowers exists instead of UA sniffing (1.0)

ack, round 3.

Comment 25

[Jeff Hammel]

Comment on attachment 637218 [details] [diff] [review]
add check if specialpowers exists instead of UA sniffing (1.0)

lgtm

Comment 26

[Joel Maher ( :jmaher ) (UTC -8)]

3rd patch landed on talos:
http://hg.mozilla.org/build/talos/rev/9c0b93b5f521

Comment 27

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: set version before checking for id which we expect to throw on xul (1.0)

could this be the last and final patch for this bug?

Comment 28

[Jeff Hammel]

Comment on attachment 637922 [details] [diff] [review]
set version before checking for id which we expect to throw on xul (1.0)

I'd probably want a comment as this is really tricky ;) I am still confused, but it looks good to me

Comment 29

[Joel Maher ( :jmaher ) (UTC -8)]

third patch is landed:
http://hg.mozilla.org/build/talos/rev/4c23320b480a

Comment 30

[Joel Maher ( :jmaher ) (UTC -8)]

bm-remote is updated, now we just need to land a new talos (next week)

Comment 31

[Joel Maher ( :jmaher ) (UTC -8)]

ok, new talos is landed, and sticking this time.

Comment 32

[Joel Maher ( :jmaher ) (UTC -8)]

it seems that the specialpowers extension is causing a performance regression (and added instability) to our numbers.  technically the problem is solved, we just need to trim it down and make it more acceptable to talos.

Comment 33

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: remove specialpowers from the talos config

sad sad day, but this has to be done for now.

Comment 34

[Jeff Hammel]

Comment on attachment 641060 [details] [diff] [review]
remove specialpowers from the talos config

Yeah, this wfm.  Too bad this didn't stick

Comment 35

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: the talos numbers after turning this off vs the numbers before we landed specialpowers.

Comment 36

[Joel Maher ( :jmaher ) (UTC -8)]

landed removal of specialpowers from the config:
http://hg.mozilla.org/build/talos/rev/9ae703f56285

Comment 37

[Joel Maher ( :jmaher ) (UTC -8)]

I forgot to make this a leave open bug, we still need to get this landed.

Comment 38

[(not currently active) Ted Mielczarek]

We could try this with the quitter extension I wrote in bug 641829 to see if it has the same problems. If so we might need to profile the browser with one of these extensions installed and see what the problem is.

Comment 39

[Joel Maher ( :jmaher ) (UTC -8)]

we are not going to use special powers, if we do anything like that it will be a very custom messagemanager script.