talos has enablePrivilege deprecated warning, need to use specialpowers

RESOLVED WONTFIX

Status

defect
RESOLVED WONTFIX
8 years ago
5 years ago

People

(Reporter: jmaher, Assigned: jmaher)

Tracking

Trunk
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 2 obsolete attachments)

(Assignee)

Description

8 years ago
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.
(Assignee)

Comment 1

7 years ago
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.
Assignee: nobody → jmaher
Status: NEW → ASSIGNED
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.
(Assignee)

Comment 3

7 years ago
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.
(Assignee)

Comment 4

7 years ago
I just realized a flaw in this, all branches use bm-remote, so we would need to deploy to all branches.
(Assignee)

Comment 5

7 years ago
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?
(Assignee)

Comment 6

7 years ago
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.
Attachment #632106 - Attachment is obsolete: true
Attachment #632313 - Flags: feedback?(jhammel)
(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?
(Assignee)

Comment 8

7 years ago
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?
(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?
(Assignee)

Comment 10

7 years ago
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

7 years ago
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.
Attachment #632313 - Flags: feedback?(jhammel) → feedback+
(Assignee)

Comment 12

7 years ago
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.
(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.
(Assignee)

Comment 14

7 years ago
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.
(Assignee)

Comment 15

7 years ago
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.
Attachment #632313 - Attachment is obsolete: true
Attachment #633572 - Flags: review?(bobbyholley+bmo)
(Assignee)

Comment 16

7 years ago
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
Attachment #633572 - Flags: review?(jhammel)
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?
Attachment #633572 - Flags: review?(ted.mielczarek)
Attachment #633572 - Flags: review?(bobbyholley+bmo)
Attachment #633572 - Flags: feedback+

Comment 18

7 years ago
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
Attachment #633572 - Flags: review?(jhammel) → review+
(Assignee)

Comment 19

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

Comment 20

7 years ago
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.
Attachment #633572 - Flags: review?(ted.mielczarek)
(Assignee)

Comment 21

7 years ago
updated to fix a couple small things with enableprivilege and allow for testing with --develop on try server!
Attachment #637096 - Flags: review?(jhammel)

Comment 22

7 years ago
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
Attachment #637096 - Flags: review?(jhammel) → review+
(Assignee)

Comment 23

7 years ago
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.
(Assignee)

Updated

7 years ago
Depends on: 768992

Comment 25

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

lgtm
Attachment #637218 - Flags: review?(jhammel) → review+
(Assignee)

Comment 27

7 years ago
could this be the last and final patch for this bug?
Attachment #637922 - Flags: review?(jhammel)

Comment 28

7 years ago
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
Attachment #637922 - Flags: review?(jhammel) → review+
(Assignee)

Comment 30

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

Comment 31

7 years ago
ok, new talos is landed, and sticking this time.
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Assignee)

Comment 32

7 years ago
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 33

7 years ago
sad sad day, but this has to be done for now.
Attachment #641060 - Flags: review?(jhammel)

Comment 34

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

Yeah, this wfm.  Too bad this didn't stick
Attachment #641060 - Flags: review?(jhammel) → review+
(Assignee)

Comment 36

7 years ago
landed removal of specialpowers from the config:
http://hg.mozilla.org/build/talos/rev/9ae703f56285
(Assignee)

Updated

7 years ago
Depends on: 772893
(Assignee)

Comment 37

7 years ago
I forgot to make this a leave open bug, we still need to get this landed.
Blocks: 911573
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.
(Assignee)

Comment 39

5 years ago
we are not going to use special powers, if we do anything like that it will be a very custom messagemanager script.
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago5 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.