Closed Bug 1016089 Opened 6 years ago Closed 3 years ago

Allow TART to control the profiler outside of talos

Categories

(Testing :: Talos, defect)

x86
macOS
defect
Not set

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: avih, Assigned: avih)

References

Details

(Keywords: ateam-talos-task)

Attachments

(1 file, 1 obsolete file)

+++ This bug was initially created as a clone of Bug #967635 +++

The first fix (bug 1010473) only prevented complete breakage outside talos, but without making sure that the profiler is actually being controlled during the run.

Profiler.js (pause/resume the profiler in talos and TART) contains the following code as part of its setup:

> netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect');

This is no longer required nor allowed for addons since Firefox 17:
https://developer.mozilla.org/en-US/Firefox/Releases/17/Site_compatibility#Security

Therefore, when running TART outside of talos, the setup fails and the rest of the code at Profiler.js becomes no-op (TART still runs and measures, but can't control the profiler nor add markers).

However, we apparently still need this code at Profiler.js to control profiler pause/resume from non privileged pages which talos loads and tests, like the tscroll pages. In order to make it work, the talos run logs show (what probably allows the privileges request to work):

> security.turn_off_all_security_so_that_viruses_can_take_over_this_computer: true

Off topic: The preference itself is either deprecated or replaced with security.enablePrivilege.enable_for_tests (also see at the Firefox 17 security notes from the above link).

My solution is to put the privileges request inside a try/catch block, such that it will work where needed (and can work - like inside talos) and wouldn't break the setup where it's not allowed to be invoked - such as during TART runs outside of talos.
This also reverts part of the patch of bug 1010473. I think it was redundant already when it landed.
Attachment #8429481 - Flags: review?(jmaher)
Comment on attachment 8429481 [details] [diff] [review]
v1 Puts the privileges request inside try/catch

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

if we change this, I want to change the other 2 files as well!

::: talos/page_load_test/tart/addon/content/Profiler.js
@@ +27,5 @@
> +      // It's used inside talos from non-privileged pages (like during tscroll),
> +      // and it works because talos disables all/most security measures.
> +      netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect');
> +    } catch (e) {
> +    }

we we do this outside of the first try, so we don't have nested try blocks?
Attachment #8429481 - Flags: review?(jmaher) → review-
Apologies, I accidentally misread and thought it's an r+ with the changes you requested.

I modified as requested (new attachment) and pushed:
https://hg.mozilla.org/build/talos/rev/2b78501af8d0

Sorry. But do please review this as well to make sure nothing slipped.
Thanks.
Attachment #8429481 - Attachment is obsolete: true
Attachment #8429584 - Flags: review?(jmaher)
Comment on attachment 8429584 [details] [diff] [review]
v2 address review comments

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

thanks for this!
Attachment #8429584 - Flags: review?(jmaher) → review+
Depends on: 1253321
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.