629197 - disable jetpack service

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Andreas Gal :gal]

This doesn't automatically mean it will get disabled, we just need a patch to do so right now.

Comment 1

[Andreas Gal :gal]

Attachment: patch

Comment 2

[Andreas Gal :gal]

Attachment: patch

Comment 3

[Andreas Gal :gal]

This seems to be the right mix. This disables the jetpack service but keeps building the code so it doesn't bit-rot.

Comment 4

[Andreas Gal :gal]

Either this bug is a hard blocker (removing it from the build, more or less), or a bug to fix up js/jetpack. Marking so.

Comment 5

[Andreas Gal :gal]

Comment on attachment 507279 [details] [diff] [review]
patch

Myk is going to test this patch and we take it from there. I could further minimize this and not change JetpackService.cpp if you prefer.

Comment 6

[Andreas Gal :gal]

Review ping. myk confirmed that they don't need js/jetpack for now.

Comment 7

[Andreas Gal :gal]

Review ping.

Comment 8

[Andreas Gal :gal]

Review ping.

Comment 9

[Andreas Gal :gal]

http://hg.mozilla.org/tracemonkey/rev/81c46a78201f

Comment 10

[Phil Ringnalda (:philor)]

Backed out again in http://hg.mozilla.org/tracemonkey/rev/a94566343ba6 rather than disabling the failing tests like I was supposed to, because I'm not really sure how you disable an xpcshell test based on a define that doesn't exist, and then a failing mochitest-chrome test joined the party, and I decided I'd rather take a look at your tryserver push than teach myself how to disable things using the tinderbox as a tryserver. The failing ones (so far) are

http://mxr.mozilla.org/mozilla-central/source/js/jetpack/tests/unit/test_jetpack.js
http://mxr.mozilla.org/mozilla-central/source/js/jetpack/tests/unit/test_jetpack_ctypes.js
http://mxr.mozilla.org/mozilla-central/source/js/jetpack/tests/unit/test_jetpack_sandbox.js
http://mxr.mozilla.org/mozilla-central/source/js/jetpack/tests/test_jetpack_crash.xul

Comment 11

[Andreas Gal :gal]

Thanks Phil I will land it again with the tests disabled.

Comment 12

[Benjamin Smedberg]

Is there a bug on js/jetpack not working? I don't see anything in the dependencies. We already blogged/publicized this API, so it's reasonably likely  that extensions are using it...

Comment 13

[Andreas Gal :gal]

Bsmedberg, the module seems unowned and needs a makeover. Myk is trying to figure out what the future of this feature is and then we will decide what needs to be done with js/jetpack.

Comment 14

[Andreas Gal :gal]

http://hg.mozilla.org/tracemonkey/rev/a6d56af51d69

Comment 15

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/81c46a78201f
http://hg.mozilla.org/mozilla-central/rev/a94566343ba6 (backout)
Note: not marking as fixed because last changeset is a backout.

Comment 16

[Andreas Gal :gal]

Landed on mozilla-central.

http://hg.mozilla.org/mozilla-central/rev/a85d6ae60d96

Comment 17

[Chris B.]

I haven't yet downloaded the 2/2/11 build, but I have TWO questions here:
  1) How does one now enable / disable Jetpack?
  2) What exactly IS Jetpack and what types of websites use it?

Comment 18

[Andreas Gal :gal]

The jetpack service will not ship with firefox and websites could never reach it. It was an optional part of a specific class of extensions.

Comment 19

[Benjamin Smedberg]

https://developer.mozilla.org/en/Jetpack_Processes and the AMO blog will need to mention this, since we publicized the API earlier.

Comment 20

[Jorge Villalobos [:jorgev] (he/him)]

(In reply to comment #19)
> https://developer.mozilla.org/en/Jetpack_Processes and the AMO blog will need
> to mention this, since we publicized the API earlier.

I think Myk is the right person for this job.

Comment 21

[Benjamin Smedberg]

Note that this API has nothing really to do with jetpack, other than jetpacks happen to use it, so I'm not sure that myk really has much to say about it.

Comment 22

[Andreas Gal :gal]

What is this API for then? And why do we call it jetpack service? In particular considering that jetpack might not want to use it in the future?

Comment 23

[Benjamin Smedberg]

As noted in email, this API allows chrome code (including jetpacks) to run code asynchronously in another process. See http://benjamin.smedbergs.us/blog/2010-12-03/running-extension-code-in-another-process/

Comment 24

[Andreas Gal :gal]

Ok, we should really stop calling this jetpack service when we re-enable it down the road. Do you want to communicate this change?

Comment 25

[Benjamin Smedberg]

Here's the thing I don't get. The Jetpack SDK is using this code in some "bad" way. But at least the simple case (using this code from chrome) works, because we have tests for that. Can we just leave the service enabled for those extensions which are using it safely, and figure out what the jetpack SDK is doing that's causing the crashes later?

Comment 26

[Andreas Gal :gal]

Not sure this is the best place to discuss this. We are not talking about some abstract problem here, or some corner case. JetpackActorCommon::RecvMessage was crashing in debug builds. That seems like a pretty central piece of your stack. It was clearly not being tested in debug builds. How do you use this service safely without that functionality? And what does it mean to use this service safetly? And who uses it? Are we talking about some extension we don't know about? How do we know its safe? All we know right now is that the code is broken, and that it is not being tested, and that nobody is actively working on it.

I attempted to fix the crashes mentioned above, but without proper tests we won't know whether that was successful. Beyond what I fixed the code does a bunch of stuff that scares the living !@#$ out of me, such as holding on to contexts in each handle. From the code its not clear to me that's safe at all, and nobody on the JS team ever reviewed this code as far as I can tell.

Comment 27

[Eric Shepherd [:sheppy]]

Can someone please update https://developer.mozilla.org/en/Jetpack_Processes to explain the state of things?

Comment 28

[Benjamin Smedberg]

It is most definitely being tested.

JetpackActorCommon::RecvMessage is definitely working in the common case: it is tested here: http://mxr.mozilla.org/mozilla-central/source/js/jetpack/tests/unit/test_jetpack.js#31

I believe that the crashes are only introduced because the Jetpack SDK is running code from some unusual configuration (perhaps with another compartment and Cu.evalInSandbox?). We have many tests which cover the original designed use, which is from JS modules and chrome code.

Comment 29

[Tony Chung [:tchung]]

Verified fix landed on hg.  Myk, is there anything around jetpack functionality needs to be verified here?

Comment 30

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #29)
> Verified fix landed on hg.  Myk, is there anything around jetpack functionality
> needs to be verified here?

Over in bug 629791, I landed a patch that disables e10s tests on host applications that don't have the jetpack service.  It'd be useful to verify that tests continue to pass on builds that contain the fix for this bug.

Comment 31

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/a6d56af51d69

Comment 32

[Eric Shepherd [:sheppy]]

Noted that the service is disabled by default and that it needs to be enabled at compile time, here:

https://developer.mozilla.org/en/Jetpack_Processes