641829 - Remove enablePrivilege from profileserver.py's index.html

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[(not currently active) Ted Mielczarek]

The index.html file we use for PGO profiling runs uses enablePrivilege so it can quit the browser. We'll need to remove that. Joel said he was going to add a quit() method to SpecialPowers, so we should probably just fix profileserver.py to add SpecialPowers to the profile it uses, and then use that.

Comment 1

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

this is not as simple as it should be.  Curretly profileServer.py uses automation.initializeProfile (which is good), but mochitest handles the special powers (code and profile insertion) in the runtests.py script.

In order for this to happen, we need to:
 - get special powers out of mochitest (either just for this test, or relocate it in the build tree)
 - add code to automation.py or somewhere to setup special powers so we don't duplicate code.

Comment 3

[(not currently active) Ted Mielczarek]

I don't really care either way. This literally just needs a quit method, so even making a duplicate seriously stripped down SpecialPowers or something with just that one method would be fine.

Comment 4

[(not currently active) Ted Mielczarek]

Attachment: add a Quitter extension for profileserver.py

This patch adds a stripped-down copy of SpecialPowers to tools/quitter that exposes just one function to content: Quitter.quit(). It felt a bit like overkill, but so did moving SpecialPowers around just to add a single method for this to use. I'm not sure if this is the best solution, but it's not terrible. I haven't tested this in Fennec yet, but I think it ought to work fine.

Comment 5

[(not currently active) Ted Mielczarek]

Doesn't seem to work in Fennec for some reason. Extension is installed properly but the object isn't available in content... Might spend a little time tracking that down since I don't want to have to fix it in the future.

Comment 6

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

Comment on attachment 535317 [details] [diff] [review]
add a Quitter extension for profileserver.py

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

this seems pretty reasonable.  I personally don't like adding yet another addon, but this seems like the most reasonable approach with what we have now.  I just want to make sure that we are only building the quitter xpi for PGO builds and other builds that use profileserver.py.

::: toolkit/toolkit-tiers.mk
@@ +266,5 @@
>  tier_platform_dirs	+= tools/codesighs
>  endif
>  
> +tier_platform_dirs	+= tools/quitter
> +

does this build quitter all the time?  don't we want it for pgo builds only?

Comment 7

[(not currently active) Ted Mielczarek]

We use profileserver for the Valgrind builds as well, I'm not sure if they're a specific build configuration I can test for.

Comment 8

[Phil Ringnalda (:philor)]

http://hg.mozilla.org/build/tools/file/97fda6e7be4a/scripts/valgrind/valgrind.sh

Comment 9

[(not currently active) Ted Mielczarek]

Yeah, that. I think we should probably just build this unconditionally. It's only one extra directory. Since we copy profileserver.py around unconditionally, it would be weird to have some supporting machinery for it not get built in certain configurations.

Comment 10

[Masatoshi Kimura [:emk]]

Any reason is this patch not landed yet?

Comment 11

[(not currently active) Ted Mielczarek]

I don't remember why. It probably needs to be rebased at this point.

Comment 12

[Masatoshi Kimura [:emk]]

Attachment: quitter

Rebased

Comment 13

[(not currently active) Ted Mielczarek]

Comment on attachment 803674 [details] [diff] [review]
quitter

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

I can't really review my own patch, but if all you've done is rebased it then you can carry forward r=jmaher from before. Have you run this past the Try server yet, or should I?

::: tools/quitter/Makefile.in
@@ +6,5 @@
> +DEPTH           = ../..
> +topsrcdir       = @top_srcdir@
> +srcdir          = @srcdir@
> +VPATH           = @srcdir@
> +relativesrcdir  = tools/quitter

This boilerplate is no longer necessary.

Comment 14

[Masatoshi Kimura [:emk]]

Attachment: quitter v1.1

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #13)
> I can't really review my own patch, but if all you've done is rebased it
> then you can carry forward r=jmaher from before.

Hm, I had to add some nontrivial changes, so requesting :jmaher for review again.

> Have you run this past the
> Try server yet, or should I?

Running on try: https://tbpl.mozilla.org/?tree=Try&rev=3cde93da5b04

> ::: tools/quitter/Makefile.in
> @@ +6,5 @@
> > +DEPTH           = ../..
> > +topsrcdir       = @top_srcdir@
> > +srcdir          = @srcdir@
> > +VPATH           = @srcdir@
> > +relativesrcdir  = tools/quitter
> 
> This boilerplate is no longer necessary.

Removed.

Comment 15

[Masatoshi Kimura [:emk]]

(In reply to Masatoshi Kimura [:emk] from comment #14)
> Running on try: https://tbpl.mozilla.org/?tree=Try&rev=3cde93da5b04

According to bug 811404, the OS X error is ignorable.

Comment 16

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

Comment on attachment 804049 [details] [diff] [review]
quitter v1.1

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

please test this on try server for all platforms, OSX does some weird stuff.

::: toolkit/toolkit.mozbuild
@@ +180,5 @@
>  
>  if CONFIG['ENABLE_MARIONETTE'] or CONFIG['MOZ_WIDGET_TOOLKIT'] not in ('gonk', 'android'):
>      add_tier_dir('platform', 'testing/marionette')
>  
> +add_tier_dir('platform', 'tools/quitter')

do we want to add this for all builds?

::: tools/quitter/install.rdf
@@ +4,5 @@
> +     xmlns:em="http://www.mozilla.org/2004/em-rdf#">
> +
> +  <Description about="urn:mozilla:install-manifest">
> +    <em:id>[e-mail]</em:id>
> +    <em:version>2011.05.25</em:version>

might as well update this date :)

@@ +7,5 @@
> +    <em:id>[e-mail]</em:id>
> +    <em:version>2011.05.25</em:version>
> +    <em:type>2</em:type>
> +
> +    <!-- Target Application this extension can install into, 

nit: trailing whitespace

@@ +21,5 @@
> +    <!-- Front End MetaData -->
> +    <em:name>Quitter</em:name>
> +    <em:description>Adds a quit method that content pages can use to quit the application.</em:description>
> +    <em:creator>Mozilla</em:creator>
> +  </Description>      

nit: trailing whitespace

::: tools/quitter/jar.mn
@@ +1,3 @@
> +quitter.jar:
> +% content quitter %content/
> +  content/contentscript.js (contentscript.js)

do you ned the QuitterObserver.js at all?

Comment 17

[Masatoshi Kimura [:emk]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/182cb77cc3e9

(In reply to Joel Maher (:jmaher) from comment #16)
> please test this on try server for all platforms, OSX does some weird stuff.

Done. https://tbpl.mozilla.org/?tree=Try&rev=a7c18f96a50f
We don't support PGO on OSX (at least officially).
http://hg.mozilla.org/build/buildbot-configs/file/97caef8e4fd6/mozilla/config.py#l73

> ::: toolkit/toolkit.mozbuild
> @@ +180,5 @@
> >  
> >  if CONFIG['ENABLE_MARIONETTE'] or CONFIG['MOZ_WIDGET_TOOLKIT'] not in ('gonk', 'android'):
> >      add_tier_dir('platform', 'testing/marionette')
> >  
> > +add_tier_dir('platform', 'tools/quitter')
> 
> do we want to add this for all builds?

See comment #9.

> ::: tools/quitter/jar.mn
> @@ +1,3 @@
> > +quitter.jar:
> > +% content quitter %content/
> > +  content/contentscript.js (contentscript.js)
> 
> do you ned the QuitterObserver.js at all?

Yes. It's listed in chrome.manifest.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/182cb77cc3e9