Closed
Bug 629197
Opened 13 years ago
Closed 13 years ago
disable jetpack service
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
blocking2.0 | --- | betaN+ |
People
(Reporter: gal, Assigned: gal)
References
Details
(Keywords: dev-doc-complete, Whiteboard: [b11][hardblocker][has patch], fixed-in-tracemonkey, [fx4-fixed-bugday] )
Attachments
(1 file, 1 obsolete file)
1.59 KB,
patch
|
shaver
:
review+
|
Details | Diff | Splinter Review |
This doesn't automatically mean it will get disabled, we just need a patch to do so right now.
Assignee | ||
Comment 1•13 years ago
|
||
Assignee: general → gal
Assignee | ||
Comment 2•13 years ago
|
||
Attachment #507276 -
Attachment is obsolete: true
Assignee | ||
Comment 3•13 years ago
|
||
This seems to be the right mix. This disables the jetpack service but keeps building the code so it doesn't bit-rot.
Assignee | ||
Comment 4•13 years ago
|
||
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.
blocking2.0: --- → final+
Whiteboard: [hardblocker]
Assignee | ||
Comment 5•13 years ago
|
||
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.
Attachment #507279 -
Flags: review?(jst)
Assignee | ||
Comment 6•13 years ago
|
||
Review ping. myk confirmed that they don't need js/jetpack for now.
Assignee | ||
Comment 7•13 years ago
|
||
Review ping.
Assignee | ||
Comment 8•13 years ago
|
||
Review ping.
Attachment #507279 -
Flags: review?(jst) → review+
Assignee | ||
Comment 9•13 years ago
|
||
http://hg.mozilla.org/tracemonkey/rev/81c46a78201f
Whiteboard: [hardblocker] → [hardblocker], fixed-in-tracemonkey
Comment 10•13 years ago
|
||
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
Whiteboard: [hardblocker], fixed-in-tracemonkey → [hardblocker]
Assignee | ||
Comment 11•13 years ago
|
||
Thanks Phil I will land it again with the tests disabled.
Updated•13 years ago
|
Whiteboard: [hardblocker] → [hardblocker][has patch]
Comment 12•13 years ago
|
||
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...
Assignee | ||
Comment 13•13 years ago
|
||
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.
blocking2.0: final+ → ---
Assignee | ||
Comment 14•13 years ago
|
||
http://hg.mozilla.org/tracemonkey/rev/a6d56af51d69
Whiteboard: [hardblocker][has patch] → [hardblocker][has patch], fixed-in-tracemonkey
Comment 15•13 years ago
|
||
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.
Assignee | ||
Updated•13 years ago
|
blocking2.0: --- → betaN+
Updated•13 years ago
|
Whiteboard: [hardblocker][has patch], fixed-in-tracemonkey → [b11][hardblocker][has patch], fixed-in-tracemonkey
Assignee | ||
Comment 16•13 years ago
|
||
Landed on mozilla-central. http://hg.mozilla.org/mozilla-central/rev/a85d6ae60d96
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Comment 17•13 years ago
|
||
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?
Assignee | ||
Comment 18•13 years ago
|
||
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•13 years ago
|
||
https://developer.mozilla.org/en/Jetpack_Processes and the AMO blog will need to mention this, since we publicized the API earlier.
Keywords: dev-doc-needed
Assignee | ||
Updated•13 years ago
|
Summary: make a patch to optionally disable js/jetpack → disable jetpack service
Comment 20•13 years ago
|
||
(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•13 years ago
|
||
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.
Assignee | ||
Comment 22•13 years ago
|
||
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•13 years ago
|
||
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/
Assignee | ||
Comment 24•13 years ago
|
||
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•13 years ago
|
||
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?
Assignee | ||
Comment 26•13 years ago
|
||
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•13 years ago
|
||
Can someone please update https://developer.mozilla.org/en/Jetpack_Processes to explain the state of things?
Comment 28•13 years ago
|
||
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•13 years ago
|
||
Verified fix landed on hg. Myk, is there anything around jetpack functionality needs to be verified here?
Status: RESOLVED → VERIFIED
Whiteboard: [b11][hardblocker][has patch], fixed-in-tracemonkey → [b11][hardblocker][has patch], fixed-in-tracemonkey, [fx4-fixed-bugday]
Comment 30•13 years ago
|
||
(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•13 years ago
|
||
cdleary-bot mozilla-central merge info: http://hg.mozilla.org/mozilla-central/rev/a6d56af51d69
Updated•13 years ago
|
Status: VERIFIED → RESOLVED
Closed: 13 years ago → 13 years ago
Comment 32•13 years ago
|
||
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
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•