JSCompartment offers a field `AddonId` that lets us easily find out which add-on owns the compartment, which is in turn useful for tracking memory usage and performance of an add-on. A few applications: about:performance, AddonWatcher, etc. Having a similar field `TabID` would let us easily find out which tab (== top-level docShell?) owns the compartment, if any, which would be equally useful for tracking performance of a webpage. Applications: about:performance, displaying a human-readable name in the slow script dialog, monitoring pages that take lots of CPU through setTimeout, etc.

I suspect that `JS_SetCompartmentPrincipals` would be a good place to inject this information. Boris, any suggestion?

Is the information not available when creating the compartment initially? Seems to me like creation of the global+compartment is the right time for this.

Note for self: we need a way to keep this e10s-safe, as the TabID will often (not always) live in the content process but be used in the parent process.

We shouldn't get into the business of storing embedder data on JSCompartment itself (addonIds were a special case, IIRC there was some reason they couldn't be totally opaque to the JSEngine). We stick embedder data on xpc::CompartmentPrivate. We basically already track the tab of each global for the purposes of setting up zones (since we try to have one zone per tab). If you follow the places where we set the zone (when creating new globals), that should tell you where to stash the tab ID. But really, maybe zones are enough, and we just need to give them a name?

Well, for stopwatch performance monitoring, we need the information to be visible to JSAPI. Currently, we use the `addonId*` as a key for add-ons and the `JSPrincipals*` for webpages, but the latter won't let us merge results per tab. I like the idea of using the Zone, if that works. Will several iframes in the same tab share a Zone? If so, we "just" need the ability to map a Zone to a top-level docShell.

Unless I'm mistaken, a Facebook Like button iframe will not share the same Zone as the rest of the tab, in which case Zone does not answer my needs. So, I'm back to my initial program: introducing a `tabId` with essentially the same mechanism as `addonId`, unless someone has a better plan.

I suspect that e10s already have some kind of globally unique id for each tab. To improve reusability, we should use this. Bill, would you know something about that?

(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #5) > Well, for stopwatch performance monitoring, we need the information to be > visible to JSAPI. Currently, we use the `addonId*` as a key for add-ons and > the `JSPrincipals*` for webpages, but the latter won't let us merge results > per tab. > > I like the idea of using the Zone, if that works. Will several iframes in > the same tab share a Zone? Yes. > If so, we "just" need the ability to map a Zone > to a top-level docShell. (In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #6) > Unless I'm mistaken, a Facebook Like button iframe will not share the same > Zone as the rest of the tab, in which case Zone does not answer my needs. > So, I'm back to my initial program: introducing a `tabId` with essentially > the same mechanism as `addonId`, unless someone has a better plan. What makes you believe this? Zones are per-tab.

See http://mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#2296

At the time we collect the performance data, why don't we just find the compartment's global? From there we can go to its window, then its docshell, and then the top-level content docshell. That then gives us all the information about the tab that's available (URL, title, etc.). I don't think we need to involve the zone. Zones are per-tab, but I'd rather not depend on that too much.

> What makes you believe this? Zones are per-tab. Ah, good to know. How reliable is that assumption? It would be annoying if performance monitoring ended up preventing the Next Big Thing in security/garbage-collection. > At the time we collect the performance data, why don't we just find the compartment's global? Right. If you're talking about when we collect the data inside the VM, we could do this through an embedder callback, and then use the toplevel docshellID as a key – I wouldn't want to store either the docshell itself (as it might die by the time we make use of the data) or the url or title (as they can change during time, while our keys should remain stable). If you're talking about when we extract the data from the VM to make it available to the rest of the world, that's too late, we would have lost valuable information by the time we reach that point. Can we be certain that a JSCompartment attached to a webpage won't change docshell during its execution? > From there we can go to its window, then its docshell, and then the top-level content docshell. That then gives us all the information about the tab that's available (URL, title, etc.). I don't think we need to involve the zone. Zones are per-tab, but I'd rather not depend on that too much.

(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #11) > Can we be certain that a JSCompartment attached to a webpage won't change > docshell during its execution? As far as I'm aware, yes.

> Ah, good to know. How reliable is that assumption? It would be annoying if performance > monitoring ended up preventing the Next Big Thing in security/garbage-collection. It could definitely change, so I'd rather we not rely on it if possible. > If you're talking about when we extract the data from the VM to make it available to the rest > of the world, that's too late, we would have lost valuable information by the time we reach > that point. Could you give more context on how this will be used? I'm having a hard time understanding what you mean. What valuable information would be lost? Are you going to be displaying information about compartments that might be dead? Once a compartment dies, it seems like we probably don't care about it anymore. But if the compartment is alive, then we should be able to get whatever information you need about the tab. The only exception that I know of would be weird cases where a content window in one tab holds on to a content window from another tab that was closed. Is that what you're concerned about here?

(In reply to Bill McCloskey (:billm) from comment #13) > > If you're talking about when we extract the data from the VM to make it available to the rest > > of the world, that's too late, we would have lost valuable information by the time we reach > > that point. > > Could you give more context on how this will be used? I'm having a hard time > understanding what you mean. What valuable information would be lost? Are > you going to be displaying information about compartments that might be > dead? Once a compartment dies, it seems like we probably don't care about it > anymore. But if the compartment is alive, then we should be able to get > whatever information you need about the tab. The only exception that I know > of would be weird cases where a content window in one tab holds on to a > content window from another tab that was closed. Is that what you're > concerned about here? The main case I have in mind right now is counting CPU usage by a webpage. If we count two frames separately and sum the result later, whenever this is actually one frame causing CPU use in another frame, we double-count the total time. To avoid this, we need to identify early that, while the VM is switching from one compartment to another one, we are still monitoring the same webpage.

Actually, Kannan suggested an alternative approach. Instead of a tabID, we can introduce a notion of hierarchy of JSCompartment, with a toplevel JSCompartment == a tab. This would be sufficient to cover all my needs.

This hierarchy already exists via the docshell associated with the globals right (modulo sandboxes)? I don't think we should duplicate it.

Well, here's the information I need somehow attached to a JSCompartment: 1. the ability to find out that another JSCompartment belongs to the same tab, so that if both compartments are on the stack, we only need to monitor the topmost; 2. some information that can be extracted from SpiderMonkey and used at a later stage to find out to which tab the JSCompartment belongs. Both pieces of data could be served by a tabID, or by a (possibly opaque) pointer to the JSCompartment attached to the topmost docshell in the tab, or by a (opaque) pointer to the topmost docshell. The Zone would also do the trick, but that looks like begging for trouble for the day Zones need to be refactored.

To detail, the embedder callback I mentioned in comment 11 would be something along the lines of void* key = rt->getPerformanceKeyForCompartment(compartment) where `key` is either a tabID or a variant thereof. This callback would be used in `js::PerformanceGroupHolder::getHashKey()`.

How about the ID of the topmost outer window (1-to-1 with docshell)? You could easily write a callback to tell you this inside the JS engine: xpc::WindowGlobalOrNull, then get the docshell, then get the topmost content docshell.

I can try and work with that.

/r/7123 - Bug 1149486 - Regrouping PerformanceStats by window;r=jandem,bholley /r/7199 - Bug 1149486 - Extracting a window title and window ID for PerformanceStats;r=mossop Pull down these commits: hg pull -r 6a669163f435fcbe948d2f26280a566c66b99a64 https://reviewboard-hg.mozilla.org/gecko/

Comment on attachment 8593984 [details] MozReview Request: bz://1149486/Yoric /r/7123 - Bug 1149486 - Regrouping PerformanceStats by window;r=jandem,bholley /r/7199 - Bug 1149486 - Extracting a window title and window ID for PerformanceStats;r=mossop Pull down these commits: hg pull -r 6a669163f435fcbe948d2f26280a566c66b99a64 https://reviewboard-hg.mozilla.org/gecko/

Here we go. Note that I have exposed the underlying window, for the sake of bug 1146945, bug 1146947, bug 1146948, bug 1152262.

https://reviewboard.mozilla.org/r/7199/#review5979 ::: toolkit/components/aboutperformance/nsPerformanceStats.cpp (Diff revision 1) > + } while(false); This do ... while(false) business strikes me as evil. Can you break it out into a separate function instead? ::: toolkit/components/aboutperformance/nsPerformanceStats.cpp (Diff revision 1) > + nsresult rv = win->GetScriptableTop(getter_AddRefs(top)); So for an inner frame the window property is actually window.top. Wouldn't it be better to use the actual window and then callers can get window.top if that is all they care about?

Comment on attachment 8593984 [details] MozReview Request: bz://1149486/Yoric /r/7123 - Bug 1149486 - Regrouping PerformanceStats by window;r=jandem,bholley /r/7199 - Bug 1149486 - Extracting a window title and window ID for PerformanceStats;r=mossop /r/7221 - Bug 1149486 - Regrouping PerformanceStats by window (feedback);r=jandem,bholley /r/7223 - Bug 1149486 - Extracting a window title and window ID for PerformanceStats (feedback);r=mossop Pull down these commits: hg pull -r d5454d95feb44e790c9bb8e9797af6ecfc965056 https://reviewboard-hg.mozilla.org/gecko/

Applied feedback (and propagated to high-level code).

https://reviewboard.mozilla.org/r/7223/#review5981 Ship It! ::: toolkit/components/aboutperformance/nsPerformanceStats.cpp (Diff revision 1) > - nsCOMPtr<nsIDOMWindow> top; > + nsCOMPtr<nsIDOMWindow> top = GetWindow(cx, c->compartment); Just call this window now it is no longer the top window. ::: toolkit/modules/PerformanceStats.jsm (Diff revision 1) > * @field {number} windowID The outer window ID of the nsIDOMWindow to which Update this to say it is the outer window ID of the top-level window.

Comment on attachment 8593984 [details] MozReview Request: bz://1149486/Yoric BTW using hg's evolve extension when updating changes for posting to review board makes for a lovely reviewing experience.

https://reviewboard.mozilla.org/r/7121/#review5999 > We have explicitly avoided adding this constructor precisely because compartments are not a safe object to work with (they can be GCed at any time, as your comment mentions). You should base this code off of JSObjects (globals, probably), from which compartments are inferred. > We have explicitly avoided adding this constructor precisely because compartments are not a safe object to work with (they can be GCed at any time, as your comment mentions). I see exactly this code in `js::AutoCompartment`, what's the difference? > You should base this code off of JSObjects (globals, probably), from which compartments are inferred. I'm not sure I understand. Are you suggesting I store a `JSObject*` in my `PerformanceStats` just to be able to perform a call to `JSAutoCompartment`? Is that any safer?

https://reviewboard.mozilla.org/r/7121/#review6003 > I don't understand why we need anything more than a single argument here (probably just cx). Everything else can be inferred from that (cx gives you current global, global gives you a compartment, compartment gives you a principal, principal tells you whether you're system or not). I can clearly get rid of the principals (which is actually not used) and `isSystem`. I imagine I could remove the compartment by using `CurrentGlobalOrNull` and `GetObjectCompartment`, but that strikes me as a bit strange – after all, since the whole point of this function is to derive a key from a `JSCompartment*`, what do we gain by hiding that `JSCompartment*` behind a series of non compositional implicit deductions?

(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #29) > > We have explicitly avoided adding this constructor precisely because compartments are not a safe object to work with (they can be GCed at any time, as your comment mentions). > I see exactly this code in `js::AutoCompartment`, what's the difference? Not sure, but AutoCompartment is only used internally and much less frequently. Probably worth asking someone who works on the GC what they think (i.e. sfink). > > You should base this code off of JSObjects (globals, probably), from which compartments are inferred. > I'm not sure I understand. Are you suggesting I store a `JSObject*` in my > `PerformanceStats` just to be able to perform a call to `JSAutoCompartment`? > Is that any safer? Well, and presumably rooting it. The object is the thing that keeps the compartment alive, and the object is also the thing that will be noticed by our hazard analysis. But again, sfink can probably tell you more of how they want it done these days.

(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #30) > https://reviewboard.mozilla.org/r/7121/#review6003 > > > I don't understand why we need anything more than a single argument here (probably just cx). Everything else can be inferred from that (cx gives you current global, global gives you a compartment, compartment gives you a principal, principal tells you whether you're system or not). > > I can clearly get rid of the principals (which is actually not used) and > `isSystem`. > > I imagine I could remove the compartment by using `CurrentGlobalOrNull` and > `GetObjectCompartment`, but that strikes me as a bit strange – after all, > since the whole point of this function is to derive a key from a > `JSCompartment*`, what do we gain by hiding that `JSCompartment*` behind a > series of non compositional implicit deductions? The safest way to interact with a JSCompartment is to be in it, since then it can't go away during a GC. In general, I think you should stop focusing on the JSCompartment, because it's a kind of dangerous object to hold onto - just store the globals you're interested in, and go directly from those globals to the docshell.

https://reviewboard.mozilla.org/r/7121/#review6009 > > We have explicitly avoided adding this constructor precisely because compartments are not a safe object to work with (they can be GCed at any time, as your comment mentions). > I see exactly this code in `js::AutoCompartment`, what's the difference? > > > You should base this code off of JSObjects (globals, probably), from which compartments are inferred. > I'm not sure I understand. Are you suggesting I store a `JSObject*` in my `PerformanceStats` just to be able to perform a call to `JSAutoCompartment`? Is that any safer? Ok, now storing a `JS::Heap<JSObject*>`. I hope that does the trick. > I can clearly get rid of the principals (which is actually not used) and `isSystem`. > > I imagine I could remove the compartment by using `CurrentGlobalOrNull` and `GetObjectCompartment`, but that strikes me as a bit strange – after all, since the whole point of this function is to derive a key from a `JSCompartment*`, what do we gain by hiding that `JSCompartment*` behind a series of non compositional implicit deductions? Done and renamed appropriately.

Comment on attachment 8593984 [details] MozReview Request: bz://1149486/Yoric /r/7123 - Bug 1149486 - Regrouping PerformanceStats by window;r=jandem,bholley /r/7199 - Bug 1149486 - Extracting a window title and window ID for PerformanceStats;r=mossop Pull down these commits: hg pull -r 188f49d102afce3cfeb3c29f2e4c2ce27c2bbd40 https://reviewboard-hg.mozilla.org/gecko/

https://reviewboard.mozilla.org/r/7121/#review6041 Oops, it looks like my use of `JS:Handle` was incorrect and that fixing it would have required all sorts of gc-related stuff that I didn't want. I fixed this by turning my JSAPI to a callback-based iterator which doesn't need to allocate anything to the heap.

Comment on attachment 8593984 [details] MozReview Request: bz://1149486/Yoric /r/7123 - Bug 1149486 - Regrouping PerformanceStats by window;r=jandem,bholley /r/7199 - Bug 1149486 - Extracting a window title and window ID for PerformanceStats;r=mossop Pull down these commits: hg pull -r 7a0bce45178c82a3cf3ad0e715baa314939b72a0 https://reviewboard-hg.mozilla.org/gecko/

Comment on attachment 8593984 [details] MozReview Request: bz://1149486/Yoric My C++ chops are pretty limited so I don't think I'm up to reviewing the C++ in this patch now. Maybe one of the others can take that part too?

Comment on attachment 8593984 [details] MozReview Request: bz://1149486/Yoric https://reviewboard.mozilla.org/r/7121/#review6075 ::: js/xpconnect/src/XPCJSRuntime.cpp:3307 (Diff revision 4) > + JSCompartment* compartment = GetObjectCompartment(global); The JS engine's notion of "system compartment" is pretty vestigial, and we shouldn't rely on it. Your code is in gecko, so if you want to check for system-principaled compartments, just do: if (nsContentUtils::IsSystemPrincipal(nsContentUtils::ObjectPrincipal())) { ... } But really, why are you checking for system principal at all? Don't you just want to (1) check if the global is associated with an addon, and then (2) check if the global belongs to a DOM window? ::: toolkit/components/aboutperformance/nsPerformanceStats.cpp:151 (Diff revision 4) > -nsPerformanceSnapshot::ImportStats(js::PerformanceStats* c) { > +nsPerformanceSnapshot::ImportStats(const js::PerformanceStats& c, JS::HandleObject maybeGlobal) { IIRC non-XPConnect Gecko code needs to use JS::Handle<JSObject*> ::: js/src/jsapi.h:1466 (Diff revision 4) > + JSAutoCompartment(JSContext* cx, JSCompartment* target); This should go away, right? ::: js/src/jsapi.cpp:300 (Diff revision 4) > + js::AutoCompartment autoCompartment(cx, compartment); I'm not convinced that it's safe to enter a compartment you get from a CompartmentsIter - I don't see anything in the GC marking code that marks the global of the compartment code is currently in. Please talk to a GC person (terrence, sfink, or jonco) about this concern - this constructor seems like a total footgun to me. Naively, the safest thing to do would be to create a stack-scoped RootedObject for the compartment's global (if it exists), and then enter that. ::: toolkit/components/aboutperformance/nsPerformanceStats.cpp:106 (Diff revision 4) > + nsCOMPtr<nsIDOMWindow> mWindow; This should probably be an nsPIDOMWindow. We generally prefer that for C++ members because it guarantees that the object is not JS-implemented (and can be safely cast to nsGlobalWindow*). ::: toolkit/components/aboutperformance/tests/browser/browser_compartments_script.js:4 (Diff revision 4) > + if ("title" in e.data) { whitespace error ::: toolkit/modules/PerformanceStats.jsm:82 (Diff revision 4) > + let win = xpcom.window.top || xpcom.window; Why the || xpcom.window? ::: toolkit/components/aboutperformance/nsPerformanceStats.cpp:33 (Diff revision 4) > + , mWindow(aWindow) How long do these things live? Do we really want to hold every window in the system alive? Wouldn't it be better to just immediately extract the title/id/etc? ::: js/src/jsapi.cpp:334 (Diff revision 4) > + if (!(*walker)(stat, global, closure)) { Rather than passing a handle to the global object on the various walker iterations, it seems like we should just enter the compartment (with the caveats above), call the walker with a JSContext\*, and let it infer the global from the current compartment as required. In general, a signature with a HandleObject and no cx is very un-idiomatic.

https://reviewboard.mozilla.org/r/7121/#review6083 > How long do these things live? Do we really want to hold every window in the system alive? Wouldn't it be better to just immediately extract the title/id/etc? Actually, we extract title and id at JS-level, just one step higher-level – and then discard the entire object. But yes, we don't need to keep the window alive so long. > The JS engine's notion of "system compartment" is pretty vestigial, and we shouldn't rely on it. Your code is in gecko, so if you want to check for system-principaled compartments, just do: > > if (nsContentUtils::IsSystemPrincipal(nsContentUtils::ObjectPrincipal())) { > ... > } > > But really, why are you checking for system principal at all? Don't you just want to (1) check if the global is associated with an addon, and then (2) check if the global belongs to a DOM window? > The JS engine's notion of "system compartment" is pretty vestigial, and we shouldn't rely on it. Ah, good to know. I don't think that's documented anywhere atm. > But really, why are you checking for system principal at all? Don't you just want to (1) check if the global is associated with an addon, and then (2) check if the global belongs to a DOM window? It's not clear to me what this is going to do in presence of code running in a XUL Window. Won't that be listed as belonging to a DOM window? > Why the || xpcom.window? I can't find the source right now, but according to my experiments, `xpcom.window.top` seemed to return `nullptr` for some toplevel windows. I'll check again. > I'm not convinced that it's safe to enter a compartment you get from a CompartmentsIter - I don't see anything in the GC marking code that marks the global of the compartment code is currently in. Please talk to a GC person (terrence, sfink, or jonco) about this concern - this constructor seems like a total footgun to me. > > Naively, the safest thing to do would be to create a stack-scoped RootedObject for the compartment's global (if it exists), and then enter that. Double-checked with sfink: `CompartmentsIter` prevents GC. > IIRC non-XPConnect Gecko code needs to use JS::Handle<JSObject*> This doesn't seem respected in m-c: https://dxr.mozilla.org/mozilla-central/search?q=HandleObject&offset=200

Comment on attachment 8593984 [details] MozReview Request: bz://1149486/Yoric /r/7123 - Bug 1149486 - Regrouping PerformanceStats by window;r=jandem,bholley /r/7199 - Bug 1149486 - Extracting a window title and window ID for PerformanceStats;r=mossop Pull down these commits: hg pull -r 05fe9b818a1921d18595370f5e3fc7713b649506 https://reviewboard-hg.mozilla.org/gecko/

https://reviewboard.mozilla.org/r/7121/#review6125 I simplified the in-JSAPI code further and got rid of `PerformanceStats`. Now we only have `PerformanceData` and every relevant data on the `JSComponent` is extracted in the xpcom-layer.

https://reviewboard.mozilla.org/r/7199/#review6177 r+ for the non-c++ pieces, please find someone more knowledgable to handle those.

https://reviewboard.mozilla.org/r/7121/#review6183 > This doesn't seem respected in m-c: https://dxr.mozilla.org/mozilla-central/search?q=HandleObject&offset=200 Aside from a handful of miscreants in widget/android, that link seems to prove exactly my point, unless I'm missing something - JS engine and XPConnect use HandleObject, everything else uses Handle<JSObject*>.

Comment on attachment 8593984 [details] MozReview Request: bz://1149486/Yoric https://reviewboard.mozilla.org/r/7121/#review6185 ::: js/src/jsapi.h:5562 (Diff revision 5) > + * Callback used to ask the embedding to determine in which > + * Performance Group a non-system compartment belongs. Typically, this > + * is used to regroup JSCompartments from several iframes from the I'd drop the "non-system" bit. ::: js/src/vm/Interpreter.cpp:414 (Diff revision 5) > + , runtime_(cx->runtime()) Why do we need to store both cx\_ and runtime\_? The latter is always inferrable from the former. ::: js/xpconnect/src/XPCJSRuntime.cpp:1785 (Diff revision 5) > + if (!compartment) { > + name.AssignLiteral("no compartment"); > + return; > + } This case can never happen - please remove this branch. ::: js/xpconnect/src/XPCJSRuntime.cpp:3339 (Diff revision 5) > + if (NS_FAILED(rv)) { I'd NS_ENSURE_SUCESS(rv, nullptr) here to get a warning. I know there was a ban on that, but it didn't stick. ::: toolkit/components/aboutperformance/nsIPerformanceStats.idl:9 (Diff revision 5) > +#include "nsIDOMWindow.idl" Hm, is this still needed? ::: toolkit/components/aboutperformance/nsIPerformanceStats.idl:44 (Diff revision 5) > + * outer window (i.e. the tab), otherwise an empty string. Empty string? ::: toolkit/components/aboutperformance/nsPerformanceStats.cpp:198 (Diff revision 5) > + if (!global) { > + return; > + } Seems like the caller should do this null-check. You could also avoid passing the Handle entirely and use xpc::CurrentWindowOrNull directly on cx. ::: toolkit/components/aboutperformance/nsPerformanceStats.cpp:227 (Diff revision 5) > + if (!global) { > + return; > + } Same here about the caller doing the null check. ::: toolkit/components/aboutperformance/nsPerformanceStats.cpp:249 (Diff revision 5) > -nsPerformanceSnapshot::ImportStats(js::PerformanceStats* c) { > +nsPerformanceSnapshot::ImportStats(JSContext* cx, const js::PerformanceData& performance) { > + JS::RootedObject global(cx, JS::CurrentGlobalOrNull(cx)); > + Probably also worth bailing out here directly if there's no global.

https://reviewboard.mozilla.org/r/7121/#review6315 > Empty string? I don't understand the question here.

Comment on attachment 8593984 [details] MozReview Request: bz://1149486/Yoric /r/7123 - Bug 1149486 - Regrouping PerformanceStats by window;r=jandem,bholley /r/7199 - Bug 1149486 - Extracting a window title and window ID for PerformanceStats;r=mossop Pull down these commits: hg pull -r b7014ccbbee46649bed1b03527acd045938b5d43 https://reviewboard-hg.mozilla.org/gecko/

(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo" - away until May 6th) from comment #45) > https://reviewboard.mozilla.org/r/7121/#review6315 > > > Empty string? > > I don't understand the question here. The comment about the default value refers to the wrong datatype.

Comment on attachment 8593984 [details] MozReview Request: bz://1149486/Yoric /r/7123 - Bug 1149486 - Regrouping PerformanceStats by window;r=jandem,bholley /r/7199 - Bug 1149486 - Extracting a window title and window ID for PerformanceStats;r=mossop Pull down these commits: hg pull -r 1347cc078206a6cb36ff6ae92b4280375b1a260b https://reviewboard-hg.mozilla.org/gecko/

https://reviewboard.mozilla.org/r/7121/#review6371 > I don't understand the question here. Ah, right. Missed my spot check.

Comment on attachment 8593984 [details] MozReview Request: bz://1149486/Yoric https://reviewboard.mozilla.org/r/7121/#review6399 Compartment mechanics look good - needs review from someone with a higher picture of this architecture. I guess that's jandem? ::: js/src/jsapi.cpp:276 (Diff revision 7) > -GetPerformanceStats(JSRuntime* rt, > +PerformanceStatsIter(JSContext* cx, PerformanceStatsIter sounds like a typename to me, rather than a function name. How about IteratePerformanceStats? ::: toolkit/components/aboutperformance/nsPerformanceStats.cpp:202 (Diff revision 7) > + nsCOMPtr<nsPIDOMWindow> top = win->GetPrivateRoot(); Hm, is this the right thing to be using? It seems to skip over docshell boundaries, which makes sense I suppose, but please check with smaug. Over

Checked with smaug.

Comment on attachment 8593984 [details] MozReview Request: bz://1149486/Yoric /r/7123 - Bug 1149486 - Regrouping PerformanceStats by window;r=jandem,bholley /r/7199 - Bug 1149486 - Extracting a window title and window ID for PerformanceStats;r=mossop Pull down these commits: hg pull -r 571ada14f32aa9b5278052765f6116a5c891c71b https://reviewboard-hg.mozilla.org/gecko/

https://reviewboard.mozilla.org/r/7121/#review6909 Thanks for all the reviews.

Jandem, do you wish to perform a final review on the JSAPI code?

Comment on attachment 8593984 [details] MozReview Request: bz://1149486/Yoric /r/7123 - Bug 1149486 - Regrouping PerformanceStats by window;r=jandem,bholley /r/7199 - Bug 1149486 - Extracting a window title and window ID for PerformanceStats;r=mossop Pull down these commits: hg pull -r c6ca00590b15c89940b2d7ad33a5c1b54eec8dee https://reviewboard-hg.mozilla.org/gecko/

Comment on attachment 8593984 [details] MozReview Request: bz://1149486/Yoric /r/7123 - Bug 1149486 - Regrouping PerformanceStats by window;r=jandem,bholley /r/7199 - Bug 1149486 - Extracting a window title and window ID for PerformanceStats;r=mossop Pull down these commits: hg pull -r 528f1c0d7630cf0fef7e45173bc8e100cd64497d https://reviewboard-hg.mozilla.org/gecko/

I generally rely on checkin-needed to land code, but this patch has been marked checkin-needed for 10 days. Has it somehow slipt past some Bugzilla filters or has the procedure changed?

I was out basically all last week and I know there were numerous issues in the mean time. We'll try to catch up in the coming days, sorry for the delay.

In the future, running mach update-uuids on interfaces you're changing will speed up the checkin process.

https://hg.mozilla.org/integration/fx-team/rev/9b60d38c552e https://hg.mozilla.org/integration/fx-team/rev/99b209f7d085

Backed out for static analysis bustage. https://treeherder.mozilla.org/logviewer.html#?job_id=3106858&repo=fx-team

Backout: https://hg.mozilla.org/integration/fx-team/rev/6973f1341ace

Comment on attachment 8593984 [details] MozReview Request: bz://1149486/Yoric /r/7123 - Bug 1149486 - Regrouping PerformanceStats by window;r=jandem,bholley Pull down this commit: hg pull -r 87e8ce4a26316dacdb72d094a175c786a0f14bdd https://reviewboard-hg.mozilla.org/gecko/

Comment on attachment 8593984 [details] MozReview Request: bz://1149486/Yoric /r/7123 - Bug 1149486 - Regrouping PerformanceStats by window;r=jandem,bholley /r/8879 - Bug 1149486 - Extracting a window title and window ID for PerformanceStats;r=mossop Pull down these commits: hg pull -r 2ec86327a1ca1a5d2f484f68dffb19194f126cbe https://reviewboard-hg.mozilla.org/gecko/

Comment on attachment 8593984 [details] MozReview Request: bz://1149486/Yoric /r/7123 - Bug 1149486 - Regrouping PerformanceStats by window;r=jandem,bholley /r/8879 - Bug 1149486 - Extracting a window title and window ID for PerformanceStats;r=mossop Pull down these commits: hg pull -r 930e806cd8e5e117fc84b6785cd5030af82d7ef5 https://reviewboard-hg.mozilla.org/gecko/

Comment on attachment 8593984 [details] MozReview Request: bz://1149486/Yoric /r/7123 - Bug 1149486 - Regrouping PerformanceStats by window;r=jandem,bholley /r/8879 - Bug 1149486 - Extracting a window title and window ID for PerformanceStats;r=mossop Pull down these commits: hg pull -r 37379c3f335cb155e3232bbbc170af7324fbee5e https://reviewboard-hg.mozilla.org/gecko/

Comment on attachment 8593984 [details] MozReview Request: bz://1149486/Yoric /r/7123 - Bug 1149486 - Regrouping PerformanceStats by window;r=jandem,bholley /r/8879 - Bug 1149486 - Extracting a window title and window ID for PerformanceStats;r=mossop Pull down these commits: hg pull -r 37379c3f335cb155e3232bbbc170af7324fbee5e https://reviewboard-hg.mozilla.org/gecko/

Comment on attachment 8593984 [details] MozReview Request: bz://1149486/Yoric /r/7123 - Bug 1149486 - Regrouping PerformanceStats by window;r=jandem,bholley /r/8879 - Bug 1149486 - Extracting a window title and window ID for PerformanceStats;r=mossop Pull down these commits: hg pull -r 7dd6863c298bec1af5a12a1501771339c6489c3f https://reviewboard-hg.mozilla.org/gecko/

Comment on attachment 8593984 [details] MozReview Request: bz://1149486/Yoric /r/7123 - Bug 1149486 - Regrouping PerformanceStats by window;r=jandem,bholley /r/8879 - Bug 1149486 - Extracting a window title and window ID for PerformanceStats;r=mossop Pull down these commits: hg pull -r fd4ab63f9e707983a1357b4fcd3a46cd237c92c3 https://reviewboard-hg.mozilla.org/gecko/

Comment on attachment 8593984 [details] MozReview Request: bz://1149486/Yoric /r/7123 - Bug 1149486 - Regrouping PerformanceStats by window;r=jandem,bholley /r/8879 - Bug 1149486 - Extracting a window title and window ID for PerformanceStats;r=mossop Pull down these commits: hg pull -r dab6bf10bbdb4d6a9c3baf7fabb32d29441acb46 https://reviewboard-hg.mozilla.org/gecko/

Comment on attachment 8593984 [details] MozReview Request: bz://1149486/Yoric /r/7123 - Bug 1149486 - Regrouping PerformanceStats by window;r=jandem,bholley /r/8879 - Bug 1149486 - Extracting a window title and window ID for PerformanceStats;r=mossop Pull down these commits: hg pull -r 81243f6d44ce7aefa712a38dd0e80440b9865bd7 https://reviewboard-hg.mozilla.org/gecko/

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=81243f6d44ce

Comment on attachment 8593984 [details] MozReview Request: bz://1149486/Yoric /r/7123 - Bug 1149486 - Regrouping PerformanceStats by window;r=jandem,bholley /r/8879 - Bug 1149486 - Extracting a window title and window ID for PerformanceStats;r=mossop Pull down these commits: hg pull -r 81243f6d44ce7aefa712a38dd0e80440b9865bd7 https://reviewboard-hg.mozilla.org/gecko/

Oh, I hadn't realized that ReviewBoard actually showed every single update. Sorry for the bugspam.

(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #73) > Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=81243f6d44ce You've got bc3 issues across the board.

Comment on attachment 8593984 [details] MozReview Request: bz://1149486/Yoric /r/7123 - Bug 1149486 - Regrouping PerformanceStats by window;r=jandem,bholley /r/8879 - Bug 1149486 - Extracting a window title and window ID for PerformanceStats;r=mossop Pull down these commits: hg pull -r d366c7a2c5f25b175a3b028594a6b297c0a1f83b https://reviewboard-hg.mozilla.org/gecko/

Weird, I was sure that it had passed. I'll try and find out what I missed.

Comment on attachment 8593984 [details] MozReview Request: bz://1149486/Yoric /r/7123 - Bug 1149486 - Regrouping PerformanceStats by window;r=jandem,bholley /r/8879 - Bug 1149486 - Extracting a window title and window ID for PerformanceStats;r=mossop Pull down these commits: hg pull -r f7d14ca8fbf8cb77bce27b2b9c741f311ba86c1a https://reviewboard-hg.mozilla.org/gecko/

So this was a real bug and an error in the test, both of which together made it accidentally pass on MacOS X only. The good news is that I'm not crazy, it did pass on Try in a previous iteration. The bad news is that I need to fix the bug, of course.

Bug 1149486 - Extracting a window title and window ID for PerformanceStats;r=mossop

Bug 1149486 - Regrouping PerformanceStats by window;r=jandem,bholley low-level Bug 1149486 - Regrouping PerformanceStats by window (feedback);r=jandem,bholley Bug 1149486 - Extracting a window title and window ID for PerformanceStats;r=mossop

Can you explain what exactly you want me to re-review?

False alert. There was a bug in or around MozReview which required patching the repo manually. Sorry about the noise.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e783cc9fc9cf

https://hg.mozilla.org/integration/fx-team/rev/98603c32c9a3 https://hg.mozilla.org/integration/fx-team/rev/da71c4aefb3a

Backed out for browser_compartments.js permafail. Please run a full Try push across all platforms and test suites before requesting checkin again. https://treeherder.mozilla.org/logviewer.html#?job_id=3266329&repo=fx-team

Backout: https://hg.mozilla.org/integration/fx-team/rev/8b2ad894ed27

Er, the patch you just pushed/backed out does not include my latest changes. It's missing a file toolkit/components/perfmonitoring/tests/browser/browser_compartments_script.js that my patch has – and this missing file is causing the permafil. I assume that this is a followup from the bug I encountered on or around ReviewBoard. Ryan, how do you wish to handle this? Should I attach my patches directly to Bugzilla?

Yes, that would be best.

Uploading here, as a workaround for the hg bug.

Let's try again.

https://hg.mozilla.org/integration/fx-team/rev/d606c1796afd https://hg.mozilla.org/integration/fx-team/rev/22242ca54fd5

https://hg.mozilla.org/mozilla-central/rev/d606c1796afd https://hg.mozilla.org/mozilla-central/rev/22242ca54fd5