Instead using a ConsoleAPIStorage written in C++ or in JS, we can expose what the console API logged, directly exposing something to the WorkerDebuggerGlobalScope. I wrote a patch that is green on try, but I want to add a proper test to the new methods exposed in WorkerDebuggerGlobalScope.webidl. This patch cannot be easily applied because there are 3 depending bugs and one of them has a security flag on.

Comment on attachment 8717238 [details] [diff] [review] console3.patch We should figure out what the plan is here in terms of wrappers before I start reviewing code.

In the test I cannot do "foo[2].arguments[0] instance of Blob" because Blob is not exposed to WorkerDebuggerGlobalScope. We can expose it, but it seems wrong to do it just for this test.

> In the test I cannot do "foo[2].arguments[0] instance of Blob" You could do Object.prototype.toString.call(foo[2].arguments[0]) and see whether you get "[object Blob]".

Comment on attachment 8720758 [details] [diff] [review] console3.patch Please have bholley review the Wrap() changes (which should likely happen in a separate bug anyway). The change as written doesn't make much sense to me, though: it's just going to _always_ use &js::CrossCompartmentWrapper::singleton, since the origin global is always one of those three, no? So why the if/else thing?

Comment on attachment 8720758 [details] [diff] [review] console3.patch Review of attachment 8720758 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/workers/RuntimeService.cpp @@ +864,5 @@ > > const js::Wrapper* wrapper = nullptr; > + if (IsDebuggerGlobal(originGlobal) || > + IsDebuggerSandbox(originGlobal) || > + IsWorkerGlobal(originGlobal)) { Yeah, this seems to kind of ignore our previous discussion on this topic, right?

> Yeah, this seems to kind of ignore our previous discussion on this topic, > right? I spoke with bz about this. I would like to see if what I'm proposing here follows the result of out discussion.

So what I was proposing was that console in particular provide something like an XrayWaiver to the debugger. I also said that given lack of Xrays an XrayWaiver is basically like a transparent CCW. But of course that's only true until you get a property, since transparency is not transitive... So either we need to always use transparent wrappers, or we need a concept of something transitive like XrayWaiver on the worker thread, or we need a way for the debugger global to explicitly take an opaque wrapper and convert it to a transparent one or some equivalent (e.g. a debugger object that lives in the target compartment and so is seeing the actual object, not the wrapper). Eddy, which of these options would the debugger actually want here?

(In reply to Boris Zbarsky [:bz] from comment #10) > So what I was proposing was that console in particular provide something > like an XrayWaiver to the debugger. I also said that given lack of Xrays an > XrayWaiver is basically like a transparent CCW. > > But of course that's only true until you get a property, since transparency > is not transitive... > > So either we need to always use transparent wrappers, or we need a concept > of something transitive like XrayWaiver on the worker thread, or we need a > way for the debugger global to explicitly take an opaque wrapper and convert > it to a transparent one or some equivalent (e.g. a debugger object that > lives in the target compartment and so is seeing the actual object, not the > wrapper). Eddy, which of these options would the debugger actually want > here? We want to be able to inspect debuggee objects in the console. That is obviously a transitive operation, since if an object has another object as property, we want to be able to inspect that object as well. My understanding is that we are primarily concerned with accessor properties, since calling a getter/setter could cause debuggee code to run. It seems to me that we could create a transitive wrapper that allows you to obtain property descriptors on a debuggee object, but does not allow you to call any functions. This would cover most cases, since such a wrapper allows you to transitively inspect all properties on an object. The only restriction is that you cannot inspect the value of an accessor property, so it becomes impossible to implement something like safeGetters. That's a significant problem on the main thread, where we have a lot of DOM objects/events, but arguably less of a problem on workers, which don't have a DOM. Alternatively, the debugger already has an API to unsafely dereference an object. On the main thread, this simply removes all wrappers from the object. In worker, the opaque wrapper we use right now currently does not allow unwrapping. We could change this so that opaque wrappers can be unsafely dereferenced, but that seems like a bigger potential attack surface then the previous suggestion?

> My understanding is that we are primarily concerned with accessor properties I think Bobby is primarily concerned with what you do with the information you get from these objects and how far you go in not trusting any of it for anything. This doesn't just mean accessor properties; if you ever branch on any information you got from an untrusted object directly, that's likely a bug. > but arguably less of a problem on workers, which don't have a DOM. But does have message events, which are arguably rather important to be able to debug, right? > but that seems like a bigger potential attack surface then the previous suggestion? It does, yes. Again, if the code interacting with the unwrapped object is paranoid enough that should be ok. Anyway, this decision is what we (mostly you and Bobby, I think) need to make before we even think about writing code...

(In reply to Boris Zbarsky [:bz] from comment #12) > > My understanding is that we are primarily concerned with accessor properties > > I think Bobby is primarily concerned with what you do with the information > you get from these objects and how far you go in not trusting any of it for > anything. This doesn't just mean accessor properties; if you ever branch on > any information you got from an untrusted object directly, that's likely a > bug. > The debugger *should* do very little with the information it gets from these objects, other than sending it back to the client that requested it. That said, I'd have to audit the code to convince myself that this is actually the case. That said, even if this code doesn't do anything insecure *right now*, its not unthinkable that some of this code will end up being rewritten in the future, in such a way that it *does* become insecure. If at all possible, I would prefer a wrapper that enforces some security variants for us, rather than an all-or-nothing approach where the debugger just unwraps the opaque wrapper in cases where it needs it. > > but arguably less of a problem on workers, which don't have a DOM. > > But does have message events, which are arguably rather important to be able > to debug, right? True, I had not considered message events. > > > but that seems like a bigger potential attack surface then the previous suggestion? > > It does, yes. Again, if the code interacting with the unwrapped object is > paranoid enough that should be ok. > > Anyway, this decision is what we (mostly you and Bobby, I think) need to > make before we even think about writing code... If we cannot use Xrays (and I don't want to consider porting Xpconnect to workers), and if we cannot simply block all function calls, then I cannot think of any way to implement the functionality we need that does not involve taking an opaque wrapper and explicitly converting it to a transparent one in those places where we need it. I am not a security expert, so the final decision should be up to either you or Bobby, but it seems like those are our two options: either we don't implement object inspection for the console in workers, or we implement it by unsafely dereferencing the object, and accept the potential security risks that that entails.

(In reply to Eddy Bruel [:ejpbruel] from comment #11) > (In reply to Boris Zbarsky [:bz] from comment #10) > > So either we need to always use transparent wrappers, or we need a concept > > of something transitive like XrayWaiver on the worker thread, or we need a > > way for the debugger global to explicitly take an opaque wrapper and convert > > it to a transparent one or some equivalent (e.g. a debugger object that > > lives in the target compartment and so is seeing the actual object, not the > > wrapper). Eddy, which of these options would the debugger actually want > > here? > > We want to be able to inspect debuggee objects in the console. That is > obviously a transitive operation, since if an object has another object as > property, we want to be able to inspect that object as well. My > understanding is that we are primarily concerned with accessor properties, > since calling a getter/setter could cause debuggee code to run. > > It seems to me that we could create a transitive wrapper that allows you to > obtain property descriptors on a debuggee object, but does not allow you to > call any functions. If an object was logged as console.log(this.location) the user should be able to right click on the object in the console -> 'Store as global variable' then run arbitrary code using the reference to that variable like `temp0.toString()`. That does go through an extra step in which we use the debugger API to assign the original object into a new global variable, so not sure if it'd be affected by this wrapper.

The first question is what the main thread devtools do. My impression is that they use the Debugger API. Is that incorrect? [12:47:24] <jorendorff> bholley: Yes. Debugger.Object is its own wrapper-like class; it has a cross-compartment edge, like CCWs. [12:47:29] <jorendorff> But completely separate. [12:48:31] <jorendorff> it's designed to be convenient for inspecting objects like a Debugger wants to [12:48:42] <jorendorff> so it's massively inconvenient for actually running code against If I'm not incorrect, that means it shouldn't matter at all whether we have an opaque security wrapper or not.

(In reply to Bobby Holley (busy) from comment #15) > The first question is what the main thread devtools do. My impression is > that they use the Debugger API. Is that incorrect? > > [12:47:24] <jorendorff> bholley: Yes. Debugger.Object is its own > wrapper-like class; it has a cross-compartment edge, like CCWs. > [12:47:29] <jorendorff> But completely separate. > [12:48:31] <jorendorff> it's designed to be convenient for inspecting > objects like a Debugger wants to > [12:48:42] <jorendorff> so it's massively inconvenient for actually running > code against > > If I'm not incorrect, that means it shouldn't matter at all whether we have > an opaque security wrapper or not. Well, the worker's global object (in which the user code runs) is still exposed directly to the debugger server, which runs in the worker debugger's global object. The reason for this is that the debugger server needs to add the worker's global as a debuggee, via a call to addDebuggee on the debugger API. The debugger API is exposed via a property on the worker debugger's global object. Once it is added as debuggee, any interaction with code in the worker's global happens via the debugger API. However, I'm not sure why that means it doesn't matter whether we have an opaque security wrapper or not? It's not hard to write code in the debugger server that uses the debugger API to execute arbitrary code in the debuggee. Wouldn't that be a potential attack surface? I don't think the debugger API provides any protection here.

(In reply to Eddy Bruel [:ejpbruel] from comment #16) > (In reply to Bobby Holley (busy) from comment #15) > > The first question is what the main thread devtools do. My impression is > > that they use the Debugger API. Is that incorrect? > > > > [12:47:24] <jorendorff> bholley: Yes. Debugger.Object is its own > > wrapper-like class; it has a cross-compartment edge, like CCWs. > > [12:47:29] <jorendorff> But completely separate. > > [12:48:31] <jorendorff> it's designed to be convenient for inspecting > > objects like a Debugger wants to > > [12:48:42] <jorendorff> so it's massively inconvenient for actually running > > code against > > > > If I'm not incorrect, that means it shouldn't matter at all whether we have > > an opaque security wrapper or not. > > Well, the worker's global object (in which the user code runs) is still > exposed directly to the debugger server, which runs in the worker debugger's > global object. The reason for this is that the debugger server needs to add > the worker's global as a debuggee, via a call to addDebuggee on the debugger > API. The debugger API is exposed via a property on the worker debugger's > global object. That doesn't seem like a problem to me. > Once it is added as debuggee, any interaction with code in the worker's > global happens via the debugger API. However, I'm not sure why that means it > doesn't matter whether we have an opaque security wrapper or not? Because interacting with the global via the debugger API means that interactions go through an entirely separate set of SM-builtin wrappers, per comment 15. > It's not > hard to write code in the debugger server that uses the debugger API to > execute arbitrary code in the debuggee. Wouldn't that be a potential attack > surface? I don't think the debugger API provides any protection here. The whole point of things like XrayWrappers is to avoid _accidentally_ executing code in the debuggee, and avoid accidentally assuming that the properties on the object are meaningful and can be trusted. They don't _prevent_ you from doing so, since you can always waive Xrays with .wrappedJSObject. IIUC, the debugger API provides these same facilities a different way: by making every property access and invocation very explicit. This makes it very obvious when you're inspecting an untrusted object and executing untrusted code, and makes it unlikely that you'd do so by accident.

Comment on attachment 8720758 [details] [diff] [review] console3.patch Review of attachment 8720758 [details] [diff] [review]: ----------------------------------------------------------------- The patch looks fairly good to me. The main reason I'm r-'ing is because we're not yet handling the opaque security wrappers right. With this change, I don't think we would ever create an opaque security wrapper. We still want to create them, but we want the debugger API to be able to unwrap them, presumably via unsafeDereference. I also found a few minor bugs, and have a few questions as well. I added those to the review. ::: dom/base/Console.cpp @@ -105,5 @@ > { > AssertIsOnOwningThread(); > MOZ_ASSERT(aConsole); > > - aConsole->RegisterConsoleCallData(this); We are no longer registering the call data here? Why not? @@ +251,5 @@ > + // keep it alive or not following this enumeration. > + enum { > + // If the object is created but it's not busy in some runnable, this is its > + // status. It can be deleted at any time. > + eNone, eNone is not a very informative name. How about eUnused instead? @@ +953,5 @@ > return NS_OK; > } > > +void > +Console::MemoryPressure() I would use a verb + subject for this method name. In it's current form, it suggests that its returning a value, rather than doing something. @@ +2234,5 @@ > + > + // We cannot delete this object now because we have to trace its JSValues > + // until the pending operation (ConsoleCallDataRunnable) is completed. > + if (callData->mStatus == ConsoleCallData::eInUse) { > + mCallDataStoragePending.AppendElement(callData); Shouldn't we also set mStatus to eToBeDeleted at this point? The comment above suggests that this is the case. Otherwise, I don't see us setting the state to eToBeDeleted anywhere else. ::: dom/base/nsGlobalWindow.cpp @@ +13692,5 @@ > > if (!mConsole) { > + RefPtr<Console> console = new Console(AsInner()); > + > + console->Initialize(aRv); I assume we no longer initialize here because we do it in the getter instead? I'm not sure I understand why. ::: dom/workers/RuntimeService.cpp @@ +869,1 @@ > wrapper = &js::CrossCompartmentWrapper::singleton; If I understand Bobby's intent correctly, we should still create an OpaqueCrossCompartmentWrapper for edges from the debugger to the debuggee (i.e. the WorkerGlobal), but the debugger API should have a way to unwrap those. ::: dom/workers/WorkerPrivate.h @@ +370,5 @@ > > void > OfflineStatusChangeEvent(JSContext* aCx, bool aIsOffline); > > + void How exactly is this method used? In particular, who are its clients? Also see my comment in Console.cpp wrt the name. ::: dom/workers/WorkerScope.cpp @@ +122,5 @@ > mWorkerPrivate->AssertIsOnWorkerThread(); > > if (!mConsole) { > + RefPtr<Console> console = new Console(nullptr); > + console->Initialize(aRv); Why do we have to initialize the console here? And why didn't we have to do so before? @@ +947,5 @@ > + > + console->RetrieveConsoleEvents(aCx, aEvents, aRv); > +} > + > +void I don't feel strongly about it, but would it be nicer if we implemented this in the same way as the message event handler? (that is, using IMPL_EVENT_HANDLER)?

(In reply to Eddy Bruel [:ejpbruel] from comment #18) > Comment on attachment 8720758 [details] [diff] [review] > console3.patch > > Review of attachment 8720758 [details] [diff] [review]: > ----------------------------------------------------------------- > > The patch looks fairly good to me. The main reason I'm r-'ing is because > we're not yet handling the opaque security wrappers right. With this change, > I don't think we would ever create an opaque security wrapper. We still want > to create them, but we want the debugger API to be able to unwrap them, > presumably via unsafeDereference. Why? Why can't we inspect the object via the debugger API (rather than opting out of the debugger API, which is what unsafeDereference does)? Isn't this what the debugger API is for?

(In reply to Bobby Holley (busy) from comment #19) > (In reply to Eddy Bruel [:ejpbruel] from comment #18) > > Comment on attachment 8720758 [details] [diff] [review] > > console3.patch > > > > Review of attachment 8720758 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > The patch looks fairly good to me. The main reason I'm r-'ing is because > > we're not yet handling the opaque security wrappers right. With this change, > > I don't think we would ever create an opaque security wrapper. We still want > > to create them, but we want the debugger API to be able to unwrap them, > > presumably via unsafeDereference. > > Why? Why can't we inspect the object via the debugger API (rather than > opting out of the debugger API, which is what unsafeDereference does)? Isn't > this what the debugger API is for? Oh, I see. We're talking past each other here. My understanding is that if you don't use unsafeDereference, the debugger's reflection APIs will operate on the wrapper for the debuggee object. Jim, can you confirm if that is accurate?

(In reply to Eddy Bruel [:ejpbruel] from comment #20) > My understanding is that if you don't use unsafeDereference, the debugger's > reflection APIs will operate on the wrapper for the debuggee object. Jim, > can you confirm if that is accurate? Per the conversation I pasted in comment 16, I believe it is not accurate.

(In reply to Bobby Holley (busy) from comment #21) > (In reply to Eddy Bruel [:ejpbruel] from comment #20) > > My understanding is that if you don't use unsafeDereference, the debugger's > > reflection APIs will operate on the wrapper for the debuggee object. Jim, > > can you confirm if that is accurate? > > Per the conversation I pasted in comment 16, I believe it is not accurate. After taking a closer look at the implementation of DebuggerObject.getOwnPropertyDescriptor, it looks like the debugger enters the compartment of the debuggee object, calls GetOwnPropertyDescriptor, and then rewraps the resulting values in the debugger's compartment. The other method in the debugger API work similarly. So it looks like Bobby is correct, and the debugger API should be able to access debuggee values regardless of whether they are wrapped or not. However, now am I am confused about what triggered the discussion on whether we should change the behavior of OpaqueCrossCompartmentWrapper to begin with. Can someone remind me what problems we ran into in practice when trying to use the console in the debugger? Bgrins, can you help here?

(In reply to Eddy Bruel [:ejpbruel] from comment #20) > My understanding is that if you don't use unsafeDereference, the debugger's > reflection APIs will operate on the wrapper for the debuggee object. Jim, > can you confirm if that is accurate? If I have an object in one compartment, and a wrapper of that object in another compartment, Debugger will create distinct Debugger.Object instances for each of those. Operations on the D.O referring to the wrapper should see the object the way the wrapper presents it; operations on the D.O referring directly to the object should see the object the way other code in its compartment would. The principle is that, since the two values - object and wrapper - behave differently, Debugger's behavior must depend on which of the two you've got. If W is a Debugger.Object referring to a wrapper, then W.unwrap() produces a Debugger.Object referring to the wrapper's referent. So you can explicitly unwrap without leaving the Debugger API behind. Here are the docs: `unwrap()` : If the referent is a wrapper that this `Debugger.Object`'s compartment is permitted to unwrap, return a `Debugger.Object` instance referring to the wrapped object. If we are not permitted to unwrap the referent, return `null`. If the referent is not a wrapper, return this `Debugger.Object` instance unchanged. The unsafeDereference method just gets rid of the D.O. You should only be doing that if you need to do something to the object that the Debugger API doesn't permit. But we'd rather widen the API than encourage the use of unsafeDereference.

(In reply to Eddy Bruel [:ejpbruel] from comment #22) > (In reply to Bobby Holley (busy) from comment #21) > > (In reply to Eddy Bruel [:ejpbruel] from comment #20) > > > My understanding is that if you don't use unsafeDereference, the debugger's > > > reflection APIs will operate on the wrapper for the debuggee object. Jim, > > > can you confirm if that is accurate? > > > > Per the conversation I pasted in comment 16, I believe it is not accurate. > > After taking a closer look at the implementation of > DebuggerObject.getOwnPropertyDescriptor, it looks like the debugger enters > the compartment of the debuggee object, calls GetOwnPropertyDescriptor, and > then rewraps the resulting values in the debugger's compartment. The other > method in the debugger API work similarly. > > So it looks like Bobby is correct, and the debugger API should be able to > access debuggee values regardless of whether they are wrapped or not. > > However, now am I am confused about what triggered the discussion on whether > we should change the behavior of OpaqueCrossCompartmentWrapper to begin > with. Can someone remind me what problems we ran into in practice when > trying to use the console in the debugger? > > Bgrins, can you help here? I just had a conversation with Andrea that made me remember the original issue. Here's how I understand it: 1. The console should be able to inspect objects in the debuggee's compartment 2. Bobby claims (correctly) that we should be able to use the debugger API wrappers for this. 3. However, our implementation of the webconsole currently uses unsafeDereference. 4. Although Debugger.Object works in conjunction with OpaqueSecurityWrapper, unsafeDereference does not. 5. It was 4. that led us to to believe that we should make OpaqueSecurityWrapper less opaque. 6. However, based on 2. we should not be using unsafeDereference in the webconsole in the first place. The question is now: Isthere is any good reason for the webconsole to use unsafeDereference that we cannot work around? If there is not, we should simply rewrite the webconsole to not use unsafeDereference, and all will be well. Needinfo'ing bgrins for the above question.

Cross-posted to dev-developer-tools: --- From memory, some of that code uses unsafeDereference because it wants to display a helpful "summary" of the object that requires information that Debugger doesn't expose otherwise. For example, Debugger provides no way to obtain the source of a RegExp object, or the date represented by a Date object. These are all straightforward extensions to Debugger, but Debugger.Object starts to look like a very fat interface. There was some discussion of actually "subclassing" Debugger.Object (i.e. making the prototype chain two elements deep) with the subclass specialized for the particular type of the referent. For example, Debugger.Object has plenty of accessors that only make sense for Function objects; those would all move off Debugger.Object.prototype and onto some Function-specific prototype object. Then it would be natural to add RegExp or Promise or Poodle-specific accessors, without polluting Debugger.Object.prototype. On the other hand, it's not like the pollution really affects anyone. It's probably fine just to stick more accessors on there. It sure would be nice to get rid of as many uses of unsafeDereference as possible.

As per a discussion on IRC, here are the main cases that the webconsole is using unsafeDereference: 1) To convert an error object to a string: https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/webconsole.js#887 2) When binding _self to another object actor during evaluation. This is happening when right clicking -> 'Store as global variable' on the output: https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/webconsole.js#1213 or when evaluating script in the variables view to modify a value: https://dxr.mozilla.org/mozilla-central/source/devtools/client/webconsole/jsterm.js#782. 3) When 'previewing' an object in the console output panel i.e. when you log `new Date()` the element you see directly in the output: https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/object.js#946 Out of those, (1) is completely breaking inspection of Error objects because it breaks the preview, and it sounds like we'll need a safe toString() extension to Debugger.Object to work around this (at least for Errors, but maybe it could be shared for Dates, Regexps, etc). (2) is breaking a couple of other features, but maybe using 'unwrap' as in Comment 23 is a workaround? (3) is basically what Jim is talking about in Comment 25 and it's not necessarily breaking inspection right now as long as the preview doesn't throw.

> > - aConsole->RegisterConsoleCallData(this); > > We are no longer registering the call data here? Why not? I rename the method to 'StoreCallData'. > > + console->Initialize(aRv); > > I assume we no longer initialize here because we do it in the getter > instead? I'm not sure I understand why. In the next patch you will see a Console::Create() method that does allocation + initialization. > > + void > > How exactly is this method used? In particular, who are its clients? Also > see my comment in Console.cpp wrt the name. This is how we call things from the main-thread when prefs change. RuntimeService receives them and broadcasts them to any WorkerPrivate obj. They, using a runnable, calls an internal method on the worker thread. From there I call console->ClearStorage(). > I don't feel strongly about it, but would it be nicer if we implemented this > in the same way as the message event handler? (that is, using > IMPL_EVENT_HANDLER)? Indeed, but Console is not managing this event handler directly. It's the WorkerDebuggerGlobalScope and I would like to avoid all the serialization and so on in case we don't have such handler.

I still using the CrossOriginWrapper. That code must go away, right?

Wrong patch.

Comment on attachment 8728843 [details] [diff] [review] console3.patch Review of attachment 8728843 [details] [diff] [review]: ----------------------------------------------------------------- You addressed almost all my review comments, so in principle this patch is good to go. The only reason I r-'d it it because you're still changing the semantics of OpaqueCrossCompartmentWrapper. As discussed on irc, this is no longer necessary. Normally, I'd r+ this with comments address, but since this is security sensitive code, I'm being more careful. If you remove the changes to OpaqueCrossCompartmentWrapper, and put the patch up for review again, I'll r+ it. ::: dom/base/Console.cpp @@ +245,5 @@ > // we're created on main thread. > Maybe<ConsoleStackEntry> mTopStackFrame; > Maybe<nsTArray<ConsoleStackEntry>> mReifiedStack; > nsCOMPtr<nsIStackFrame> mStack; > The comments here below are just suggestions. Feel free to ignore them if you are satisfied with the current phrasing. @@ +249,5 @@ > > + // mStatus is about the lifetime of this object. Console must take care of > + // keep it alive or not following this enumeration. > + enum { > + // If the object is created but it's not busy in some runnable, this is its "is owned by some runnable"? @@ +253,5 @@ > + // If the object is created but it's not busy in some runnable, this is its > + // status. It can be deleted at any time. > + eUnused, > + > + // When a ConsoleCallData is taken by a runnable and sent to different "when a runnable takes ownership of"? @@ +257,5 @@ > + // When a ConsoleCallData is taken by a runnable and sent to different > + // thread, this is its status. Console cannot delete it at this time. > + eInUse, > + > + // When we are in the previous status, and the console decides to delete "when a runnable owns this ConsoleCallData, we can't delete it directly. instead, we"? ::: dom/workers/RuntimeService.cpp @@ +837,5 @@ > > JSObject* originGlobal = js::GetGlobalForObjectCrossCompartment(obj); > > const js::Wrapper* wrapper = nullptr; > + if (IsDebuggerGlobal(originGlobal) || We agreed that your code should not have to change the semantics for OpaqueCrossCompartmentWrapper (those places where webconsole.js runs into the wrapper because it uses unsafeDereference should be rewritten by extending the debugger API), so this code should go away.

Comment on attachment 8728963 [details] [diff] [review] console3.patch Review of attachment 8728963 [details] [diff] [review]: ----------------------------------------------------------------- LGTM

Backed out for mochitest failures. Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/3f3abdbf2d06 Push with failures (child push runs Tier 1 which fail): https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=50c03319c341 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=23567438&repo=mozilla-inbound 18:21:36 INFO - 716 INFO TEST-PASS | dom/base/test/test_change_policy.html | no-referrer-unsafe-urlunsafe-url tests have to be performed. 18:21:36 INFO - 717 INFO TEST-PASS | dom/base/test/test_change_policy.html | unsafe-url (changed) with no-referrer first --- full (http://mochi.test:8888/tests/dom/base/test/referrer_change_server.sjs?action=generate-policy-test2&policy=no-referrer&name=no-referrer-unsafe-url&newPolicy=unsafe-url) 18:21:36 WARNING - TEST-UNEXPECTED-FAIL | dom/base/test/test_change_policy.html | application terminated with exit code 11 18:21:36 INFO - runtests.py | Application ran for: 0:12:02.615364 18:21:36 INFO - zombiecheck | Reading PID log: /tmp/tmpqrSeT4pidlog 18:21:36 INFO - mozcrash Downloading symbols from: https://queue.taskcluster.net/v1/task/fXW5vLwURcmtl_ZCgupd9w/artifacts/public/build/target.crashreporter-symbols.zip 18:21:48 INFO - mozcrash Copy/paste: /usr/local/bin/linux64-minidump_stackwalk /tmp/tmp1p2RQp.mozrunner/minidumps/53cb60f3-70a7-b0aa-506e2afd-18428875.dmp /tmp/tmpGakT3j 18:22:04 INFO - mozcrash Saved minidump as /home/worker/workspace/build/blobber_upload_dir/53cb60f3-70a7-b0aa-506e2afd-18428875.dmp 18:22:04 INFO - mozcrash Saved app info as /home/worker/workspace/build/blobber_upload_dir/53cb60f3-70a7-b0aa-506e2afd-18428875.extra 18:22:04 WARNING - PROCESS-CRASH | dom/base/test/test_change_policy.html | application crashed [@ mozilla::dom::ConsoleCallData::~ConsoleCallData()] 18:22:04 INFO - Crash dump filename: /tmp/tmp1p2RQp.mozrunner/minidumps/53cb60f3-70a7-b0aa-506e2afd-18428875.dmp 18:22:04 INFO - Operating system: Linux 18:22:04 INFO - 0.0.0 Linux 3.13.0-79-generic #123-Ubuntu SMP Fri Feb 19 14:27:58 UTC 2016 x86_64 18:22:04 INFO - CPU: amd64 18:22:04 INFO - family 6 model 62 stepping 4 18:22:04 INFO - 1 CPU 18:22:04 INFO - Crash reason: SIGSEGV 18:22:04 INFO - Crash address: 0x0 18:22:04 INFO - Thread 43 (crashed) 18:22:04 INFO - 0 libxul.so!mozilla::dom::ConsoleCallData::~ConsoleCallData() [Console.cpp:50c03319c341 : 278 + 0x24] 18:22:04 INFO - rbx = 0x00007fc67255e440 r12 = 0x00007fc66998d700 18:22:04 INFO - r13 = 0x00007fc658aa4a50 r14 = 0x00000000000002e6 18:22:04 INFO - r15 = 0x00007fc659a02028 rip = 0x00007fc698f3e06d 18:22:04 INFO - rsp = 0x00007fc655cfe330 rbp = 0x00007fc655cfe340 18:22:04 INFO - Found by: given as instruction pointer in context 18:22:04 INFO - 1 libxul.so!mozilla::dom::ConsoleCallData::Release() [Console.cpp:50c03319c341 : 85 + 0x4] 18:22:04 INFO - rbx = 0x00007fc67255e440 r12 = 0x00007fc66998d700 18:22:04 INFO - r13 = 0x00007fc658aa4a50 r14 = 0x00000000000002e6 18:22:04 INFO - r15 = 0x00007fc659a02028 rip = 0x00007fc698f3e1cb 18:22:04 INFO - rsp = 0x00007fc655cfe350 rbp = 0x00007fc655cfe360 18:22:04 INFO - Found by: call frame info 18:22:04 INFO - 2 libxul.so!nsTArray_Impl<RefPtr<mozilla::dom::ConsoleCallData>, nsTArrayInfallibleAllocator>::RemoveElementsAt(unsigned long, unsigned long) [nsTArray.h:50c03319c341 : 523 + 0xf] 18:22:04 INFO - rbx = 0x00007fc658aa4a48 r12 = 0x00007fc6593efef8 18:22:04 INFO - r13 = 0x00007fc658aa4a50 r14 = 0x00000000000002e6 18:22:04 INFO - r15 = 0x00007fc659a02028 rip = 0x00007fc698f3e29f 18:22:04 INFO - rsp = 0x00007fc655cfe370 rbp = 0x00007fc655cfe3a0 18:22:04 INFO - Found by: call frame info 18:22:04 INFO - 3 libxul.so!mozilla::dom::Console::Shutdown() [Console.cpp:50c03319c341 : 934 + 0xb] 18:22:04 INFO - rbx = 0x00007fc6593efe40 r12 = 0x00007fc6593efe40 18:22:04 INFO - r13 = 0x00007fc6593efe80 r14 = 0x00000000000002e6 18:22:04 INFO - r15 = 0x00007fc659a02028 rip = 0x00007fc698f40aac 18:22:04 INFO - rsp = 0x00007fc655cfe3b0 rbp = 0x00007fc655cfe3f0 18:22:04 INFO - Found by: call frame info 18:22:04 INFO - 4 libxul.so!mozilla::dom::Console::cycleCollection::Unlink(void*) [Console.cpp:50c03319c341 : 802 + 0x7] 18:22:04 INFO - rbx = 0x00007fc6593efe40 r12 = 0x00007fc6593efe40 18:22:04 INFO - r13 = 0x00007fc6593efe80 r14 = 0x00000000000002e6 18:22:04 INFO - r15 = 0x00007fc659a02028 rip = 0x00007fc698f40cb7 18:22:04 INFO - rsp = 0x00007fc655cfe400 rbp = 0x00007fc655cfe420 18:22:04 INFO - Found by: call frame info 18:22:04 INFO - 5 libxul.so!nsCycleCollector::CollectWhite() [nsCycleCollector.cpp:50c03319c341 : 3326 + 0x8] 18:22:04 INFO - rbx = 0x00007fc669a25000 r12 = 0x0000000000000000 18:22:04 INFO - r13 = 0x0000000000000002 r14 = 0x00000000000002e6 18:22:04 INFO - r15 = 0x00007fc659a02028 rip = 0x00007fc698474d8c 18:22:04 INFO - rsp = 0x00007fc655cfe430 rbp = 0x00007fc655cfe4e0 18:22:04 INFO - Found by: call frame info 18:22:04 INFO - 6 libxul.so!nsCycleCollector::Collect(ccType, js::SliceBudget&, nsICycleCollectorListener*, bool) [nsCycleCollector.cpp:50c03319c341 : 3672 + 0x7] 18:22:04 INFO - rbx = 0x00007fc669a25000 r12 = 0x00007fc655cfe580 18:22:04 INFO - r13 = 0x0000000000000000 r14 = 0x0000000000000001 18:22:04 INFO - r15 = 0x0000000000000001 rip = 0x00007fc69847501c 18:22:04 INFO - rsp = 0x00007fc655cfe4f0 rbp = 0x00007fc655cfe560 18:22:04 INFO - Found by: call frame info 18:22:04 INFO - 7 libxul.so!nsCycleCollector_collect(nsICycleCollectorListener*) [nsCycleCollector.cpp:50c03319c341 : 4143 + 0x1e] 18:22:04 INFO - rbx = 0x00007fc6594aa580 r12 = 0x0000000000000000 18:22:04 INFO - r13 = 0x0000000000000003 r14 = 0x0000000000000001 18:22:04 INFO - r15 = 0x0000000000000000 rip = 0x00007fc6984752ff 18:22:04 INFO - rsp = 0x00007fc655cfe570 rbp = 0x00007fc655cfe5b0 18:22:04 INFO - Found by: call frame info 18:22:04 INFO - 8 libxul.so!AutoNotifyGCActivity::~AutoNotifyGCActivity [jsgc.cpp:50c03319c341 : 1638 + 0x10] 18:22:04 INFO - rbx = 0x00007fc655cfe600 r12 = 0x00007fc655cfe830 18:22:04 INFO - r13 = 0x0000000000000003 r14 = 0x0000000000000001 18:22:04 INFO - r15 = 0x0000000000000000 rip = 0x00007fc69b079c6d 18:22:04 INFO - rsp = 0x00007fc655cfe5c0 rbp = 0x00007fc655cfe5f0 18:22:04 INFO - Found by: call frame info 18:22:04 INFO - 9 libxul.so!js::gc::GCRuntime::gcCycle(bool, js::SliceBudget&, JS::gcreason::Reason) [jsgc.cpp:50c03319c341 : 6343 + 0x14] 18:22:04 INFO - rbx = 0x0000000000000000 r12 = 0x00007fc655cfe830 18:22:04 INFO - r13 = 0x0000000000000003 r14 = 0x0000000000000001 18:22:04 INFO - r15 = 0x0000000000000000 rip = 0x00007fc69b0c9d07 18:22:04 INFO - rsp = 0x00007fc655cfe600 rbp = 0x00007fc655cfe6b0 18:22:04 INFO - Found by: call frame info 18:22:04 INFO - 10 libxul.so!js::gc::GCRuntime::collect(bool, js::SliceBudget, JS::gcreason::Reason) [jsgc.cpp:50c03319c341 : 6428 + 0xe] 18:22:04 INFO - rbx = 0x00007fc6715ae430 r12 = 0x0000000000000001 18:22:04 INFO - r13 = 0x00007fc6715ae000 r14 = 0x0000000000000003 18:22:04 INFO - r15 = 0x0000000000000000 rip = 0x00007fc69b0ca062 18:22:04 INFO - rsp = 0x00007fc655cfe6c0 rbp = 0x00007fc655cfe820 18:22:04 INFO - Found by: call frame info 18:22:04 INFO - 11 libxul.so!js::gc::GCRuntime::gc(JSGCInvocationKind, JS::gcreason::Reason) [jsgc.cpp:50c03319c341 : 6486 + 0x1c] 18:22:04 INFO - rbx = 0x00007fc659d34800 r12 = 0x00007fc6715ae000 18:22:04 INFO - r13 = 0x0000000000000001 r14 = 0x00007fc659d34800 18:22:04 INFO - r15 = 0x00007fc655cfe9e0 rip = 0x00007fc69b0cba2c 18:22:04 INFO - rsp = 0x00007fc655cfe830 rbp = 0x00007fc655cfe870 18:22:04 INFO - Found by: call frame info 18:22:04 INFO - 12 libxul.so!js::DestroyContext(JSContext*, js::DestroyContextMode) [jscntxt.cpp:50c03319c341 : 181 + 0x13] 18:22:04 INFO - rbx = 0x00007fc659d34800 r12 = 0x00007fc6715ae000 18:22:04 INFO - r13 = 0x0000000000000001 r14 = 0x00007fc659d34800 18:22:04 INFO - r15 = 0x00007fc655cfe9e0 rip = 0x00007fc69b058bab 18:22:04 INFO - rsp = 0x00007fc655cfe880 rbp = 0x00007fc655cfe8e0 18:22:04 INFO - Found by: call frame info 18:22:04 INFO - 13 libxul.so!WorkerThreadPrimaryRunnable::Run [RuntimeService.cpp:50c03319c341 : 2664 + 0x7] 18:22:04 INFO - rbx = 0x00007fc658e31520 r12 = 0x00007fc6715ae000 18:22:04 INFO - r13 = 0x00007fc658d02000 r14 = 0x00007fc659d34800 18:22:04 INFO - r15 = 0x00007fc655cfe9e0 rip = 0x00007fc699cec510 18:22:04 INFO - rsp = 0x00007fc655cfe8f0 rbp = 0x00007fc655cfec40 18:22:04 INFO - Found by: call frame info 18:22:04 INFO - 14 libxul.so!nsThread::ProcessNextEvent(bool, bool*) [nsThread.cpp:50c03319c341 : 994 + 0x11] 18:22:04 INFO - rbx = 0x00007fc67255e080 r12 = 0x0000000000000000 18:22:04 INFO - r13 = 0x00007fc658b78d00 r14 = 0x00007fc655cfecef 18:22:04 INFO - r15 = 0x00007fc655cfec70 rip = 0x00007fc6984cf1a7 18:22:04 INFO - rsp = 0x00007fc655cfec50 rbp = 0x00007fc655cfecd0 18:22:04 INFO - Found by: call frame info 18:22:04 INFO - 15 libxul.so!NS_ProcessNextEvent(nsIThread*, bool) [nsThreadUtils.cpp:50c03319c341 : 297 + 0xc] 18:22:04 INFO - rbx = 0x00007fc658b78d00 r12 = 0x00007fc658476da0 18:22:04 INFO - r13 = 0x00007fc658b78da8 r14 = 0x0000000000000501 18:22:04 INFO - r15 = 0x00007fc658b78da0 rip = 0x00007fc6984f6413 18:22:04 INFO - rsp = 0x00007fc655cfece0 rbp = 0x00007fc655cfed00 18:22:04 INFO - Found by: call frame info 18:22:04 INFO - 16 libxul.so!mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) [MessagePump.cpp:50c03319c341 : 332 + 0xa] 18:22:04 INFO - rbx = 0x00007fc658b78d80 r12 = 0x00007fc658476da0 18:22:04 INFO - r13 = 0x00007fc658b78da8 r14 = 0x0000000000000501 18:22:04 INFO - r15 = 0x00007fc658b78da0 rip = 0x00007fc6987b16e2 18:22:04 INFO - rsp = 0x00007fc655cfed10 rbp = 0x00007fc655cfed60 18:22:04 INFO - Found by: call frame info 18:22:04 INFO - 17 libxul.so!MessageLoop::RunInternal() [message_loop.cc:50c03319c341 : 234 + 0x16] 18:22:04 INFO - rbx = 0x00007fc658476da0 r12 = 0x00007fc658476da0 18:22:04 INFO - r13 = 0x00007fc67255e0a0 r14 = 0x0000000000000501 18:22:04 INFO - r15 = 0x00000000000005bf rip = 0x00007fc69878c65f 18:22:04 INFO - rsp = 0x00007fc655cfed70 rbp = 0x00007fc655cfeda0 18:22:04 INFO - Found by: call frame info 18:22:04 INFO - 18 libxul.so!MessageLoop::Run() [message_loop.cc:50c03319c341 : 227 + 0x7] 18:22:04 INFO - rbx = 0x00007fc658476da0 r12 = 0x00007fc658476da0 18:22:04 INFO - r13 = 0x00007fc67255e0a0 r14 = 0x0000000000000501 18:22:04 INFO - r15 = 0x00000000000005bf rip = 0x00007fc69878c686 18:22:04 INFO - rsp = 0x00007fc655cfedb0 rbp = 0x00007fc655cfede0 18:22:04 INFO - Found by: call frame info 18:22:04 INFO - 19 libxul.so!nsThread::ThreadFunc(void*) [nsThread.cpp:50c03319c341 : 396 + 0x7] 18:22:04 INFO - rbx = 0x00007fc67255e080 r12 = 0x00007fc658476da0 18:22:04 INFO - r13 = 0x00007fc67255e0a0 r14 = 0x0000000000000501 18:22:04 INFO - r15 = 0x00000000000005bf rip = 0x00007fc6984cc931 18:22:04 INFO - rsp = 0x00007fc655cfedf0 rbp = 0x00007fc655cfee40 18:22:04 INFO - Found by: call frame info 18:22:04 INFO - 20 libnspr4.so!_pt_root [ptthread.c:50c03319c341 : 216 + 0x6] 18:22:04 INFO - rbx = 0x00007fc66998d700 r12 = 0x0000000000000000 18:22:04 INFO - r13 = 0x00007fc655cff700 r14 = 0x00000000000005bf 18:22:04 INFO - r15 = 0x00000000000005bf rip = 0x00007fc6a4deaef4 18:22:04 INFO - rsp = 0x00007fc655cfee50 rbp = 0x00007fc655cfeea0 18:22:04 INFO - Found by: call frame info 18:22:04 INFO - 21 libpthread-2.15.so + 0x7e99 18:22:04 INFO - rbx = 0x0000000000000000 r12 = 0x00007ffcd521d048 18:22:04 INFO - r13 = 0x00007fc655cff9c0 r14 = 0x0000000000000000 18:22:04 INFO - r15 = 0x0000000000000003 rip = 0x00007fc6a625ae9a 18:22:04 INFO - rsp = 0x00007fc655cfeeb0 rbp = 0x0000000000000000 18:22:04 INFO - Found by: call frame info

Before this set of patches we were storing Console events just in nsIConsoleStorageAPI. Now we dispatch notifications to a Worker Devtool Actor and we also expose old the events via a couple of methods. All of this happens in the WorkerDebuggerGlobalScope. What I wanted to do in this patch is to select the right JS Compartment for the ConsoleEvent. We don't need to use xpc::PrivilegedJunkScope() all the times (and we cannot actually use it in workers) and I suspect that the global scope is good enough here.

Comment on attachment 8730141 [details] [diff] [review] Bug 1246091 - patch 7/7 Correct JS compartment for ConsoleCallData Stack This patch needs a commit message. I'm a little confused by the fact that we're passing mConsoleEventHandler->Callable() for an argument called aGlobal. That makes absolutely no sense, and is presumably a bug in one of the earlier patches here; that call is not in the tree. r- until it's clear to me what compartment we're actually entering and what it has to do with the object we're trying to create. Which isn't even an "event" in the normal (dom::Event) sense, so it might be better to call it something else....

Comment on attachment 8730136 [details] [diff] [review] Bug 1246091 - patch 4/7 - Expose ConsoleCallData to WorkerDebuggerGlobalScope, r=ejpbruel >+ PopulateSequenceArguments(Sequence<JS::Value>& aSequence) const This is more of a PopulateArgumentsSequence, no? >+Console::NotifyHandler(JSContext* aCx, JS::Handle<JSObject*> aGlobal, >+ const Sequence<JS::Value>& aArguments, >+ ConsoleCallData* aCallData) const At this point ConsoleCallData owns/traces its stuff even before ConsoleCallDataRunnable::Dispatch is called, right? >+ if (NS_WARN_IF(!PopulateEvent(aCx, mConsoleEventHandler->Callable(), I think it's pretty confusing to talk about an "event handler" and "events" when these are not normal DOM event handlers and DOM events. Please come up with a better name? >+ ErrorResult rv; >+ JS::Rooted<JS::Value> ignored(aCx); >+ mConsoleEventHandler->Call(value, &ignored, rv); >+ rv.SuppressException(); How about: JS::Rooted<JS::Value> ignore(aCx); mConsoleEventHandler->Call(value, &ignored); ? That overload will handle the ErrorResult bits for you. Also, we could add a callback type that looks like "void (any arg)" for this if we wanted to... I guess this is OK, though. >+Console::RetrieveConsoleEvents(JSContext* aCx, nsTArray<JS::Value>& aEvents, >+ Sequence<JS::Value> sequence; ... >+ SequenceRooter<JS::Value> arguments(aCx, &sequence); The SequenceRooter should be on the line right after the sequence, before you go and start doing stuff with it. >+WorkerDebuggerGlobalScope::RetrieveConsoleEvents(JSContext* aCx, >+ mWorkerPrivate->GetOrCreateGlobalScope(aCx)->GetConsole(aRv); GetOrCreateGlobalScope can fail and return null, no? Can we really have a situation in which the WorkerDebuggerGlobalScope exists but the normal worker scope does not? If so, you need to null-check and throw here. If not, use the infallible getter that does not need a JSContext. >+WorkerDebuggerGlobalScope::SetConsoleEventHandler(JSContext* aCx, Same here. r=me with the above fixed.

> At this point ConsoleCallData owns/traces its stuff even before > ConsoleCallDataRunnable::Dispatch is called, right? Right. We register ConsoleCallData obj in its CTOR. > GetOrCreateGlobalScope can fail and return null, no? Can we really have a > situation in which the WorkerDebuggerGlobalScope exists but the normal > worker scope does not? If so, you need to null-check and throw here. If > not, use the infallible getter that does not need a JSContext. We have this common pattern everywhere in this file. I'm fixing it in a separate bug and mark it as a blocker for this one. What about the other patch where you give me r-? Can you tell me if I have to do some changes?

Ah. Well, a good start would be renaming aGlobal, if you're going to pass non-globals to it (and this is a problem in part 4, actually, so please fix that there). Name it based on what it really is/does. In the last part, rename aStackGlobal too, probably, since that's not what that thing is. I mean, fundamentally what you're trying to do in that patch is have a way to create the message object in a given compartment (PrivilegedJunkScope()) in some cases; that's aStackGlobal. If that compartment is not provided, you want to fall back to , but fall back to the other thing if it's not provided. What _is_ that other thing? It's in part 3, not part 4, looks like. And it's used for the compartment to do the the various argument processing in; as in, it's the global our original values live in. But why do we need to enter a compartment there at all? Because we're using AutoSafeJSContext. But in the caller. Why are we doing that in the caller, exactly? It's bizarre to call a function with a JSContext* in some random compartment and lots of JS value/object arguments in another compartment, as part 3 does. Please don't do that.

Comment on attachment 8731780 [details] [diff] [review] Bug 1246091 - patch 7/7 Correct JS compartment for ConsoleCallData Stack >+ PreDispatch(JSContext* aCx) override ... >+ JS::Rooted<JSObject*> global(aCx, JS::CurrentGlobalOrNull(aCx)); ... > JSAutoCompartment ac(aCx, global); That doesn't make any sense. It's already in that compartment, if CurrentGlobalOrNull returned that object. In general, what is the relationship between the JSContext that we now pass to Dispatch and the object we used to pass? Is the JSContext guaranteed to be same-compartment as the old argument? I can't tell from this patch.... Why do we pass anything at all? This stuff really needs documentation. Lots of documentation. >+ if (NS_WARN_IF(!PopulateEvent(aCx, argumentsScope, aArguments, PopulateEvent still needs a different name. I thought we had decided to just pass aCx in the compartment of argumentsScope (which it is here, of course) instead of passing in argumentsScope here. >+ JSAutoCompartment ac(aCx, aArgumentsScope); Makes no sense; the point was to put aCx in that compartment in the caller. We also agreed on some clear documentation about the meaning of the arguments to "PopulateEvent", which is missing from this patch.

Comment on attachment 8732497 [details] [diff] [review] Bug 1246091 - patch 7/7 Correct JS compartment for ConsoleCallData Stack You need to reset mGlobal to null in CleanUpJSObjects(), right? >+ mConsole->ProcessCallData(mCallData, aCx, values); Sort of weird to have aCx as something other than the first argument. Please fix. >+ if (NS_WARN_IF(!PopulateEventObjectInTheTargetScope(aCx, aArguments, Can we please stop calling it an "event"? I've asked for that numerous times in this bug. Please just do it. >+ // targetScope is the destination scope and value will be populate in its "populated"? > NotifyHandler(JSContext* aCx, Document that aCx and aArguments must be same-compartment here. >+ // JS Compartement and populate the ConsoleEvent object (aValue) in the s/Compartement/compartment/ (note the spelling change!) Also, s/populate/populates/ And please, stop calling it an event... :( r=me with that

https://hg.mozilla.org/integration/mozilla-inbound/rev/871c95cf3e40 https://hg.mozilla.org/integration/mozilla-inbound/rev/25dff31b92ef https://hg.mozilla.org/integration/mozilla-inbound/rev/01b1b2a0b17c https://hg.mozilla.org/integration/mozilla-inbound/rev/30134b3e46fa https://hg.mozilla.org/integration/mozilla-inbound/rev/9a1d32975c78 https://hg.mozilla.org/integration/mozilla-inbound/rev/94c7a77724d3 https://hg.mozilla.org/integration/mozilla-inbound/rev/cb19fc3357e2

Going to fix the rooting hazard this caused. For what it's worth, PopulateConsoleNotificationInTheTargetScope might have been a better name....

https://hg.mozilla.org/mozilla-central/rev/871c95cf3e40 https://hg.mozilla.org/mozilla-central/rev/25dff31b92ef https://hg.mozilla.org/mozilla-central/rev/01b1b2a0b17c https://hg.mozilla.org/mozilla-central/rev/30134b3e46fa https://hg.mozilla.org/mozilla-central/rev/9a1d32975c78 https://hg.mozilla.org/mozilla-central/rev/94c7a77724d3 https://hg.mozilla.org/mozilla-central/rev/cb19fc3357e2 https://hg.mozilla.org/mozilla-central/rev/ba45f257a090 https://hg.mozilla.org/mozilla-central/rev/7b617908d543