Closed Bug 754771 Opened 12 years ago Closed 12 years ago

Add identifying information to all system compartments

Categories

(Core :: XPConnect, defect)

Product:
Core
Component:
XPConnect
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla15

People

(Reporter: nmaier, Assigned: nmaier)

References

Details

(Whiteboard: [MemShrink:P1])

Attachments

(2 files, 5 obsolete files)

Resulting about:memory from comment 30
75.16 KB, text/plain
Details
Patch v3, add location information for jsm, jscomponents, xul, xbl, inner windows
11.72 KB, patch
bzbarsky
: review+
Ms2ger
: feedback+
Details | Diff | Splinter Review 
Corresponding to bug 673331, add identifying information to js code module compartments, now that compartment-per-global was landed and such information actually has meaning.
An implementation should be pretty trivial, as it only requires setting CompartmentPrivate.location after the module was successfully loaded.

The benefit would be to enable developers (add-on authors) to see what modules are loaded at any time. Not only can developers check the memory stats (and potential memory leaks or places to optimize) but developers may also check if their code modules gracefully "die" when Cu.unload'ing, which is a common thing to do in bootstrapped (restartless, non-sdk) add-ons.
Attached patch patch v1, add the location (obsolete) — — Splinter Review 
Working solution.
I did not put in additional NULL checks, as similar code in other places does not perform such checks as well. Each global should have a compartment, and each compartment should have a private AFAIK.

The only "draw-back" is that about:compartments/:memory may get somewhat more "noisy", especially with many add-on provided js code modules.
Attachment #623591 - Flags: review?(mrbkap)
Nils, can you show a before/after comparison of about:memory with an appropriate add-on installed?
Nicholas, you don't need to install add-ons to see the effect. Firefox comes with quite a bunch of js code modules.
Here is an "after": http://pastebin.mozilla.org/1637668
Compare that with a current Nightly + vanilla profile.
Mmm, I should have asked for about:compartments instead! :)  Looks like ~30 compartments get names.  I'm a little worried about how much this will increase amount of memory used to generate about:memory.  Bug 752381 deliberately merged all the System Principal compartments after CPG landed, because there were 100s and it was totally distorting the measurements.

Another possibility is to only show the module name in about:compartments.  If the primary use case is checking whether the module has been loaded, that would suffice.  (In fact, about:compartments is much easier than about:memory for that.)  And then about:memory wouldn't be affected.
(In reply to Nicholas Nethercote [:njn] from comment #4)
> Mmm, I should have asked for about:compartments instead! :)  Looks like ~30
> compartments get names.  I'm a little worried about how much this will
> increase amount of memory used to generate about:memory.  Bug 752381
> deliberately merged all the System Principal compartments after CPG landed,
> because there were 100s and it was totally distorting the measurements.

That was about:memory?verbose, of course, not about:memory, just if anybody else wonders.
Actually about 40 will be named (about:compartments?verbose):
http://pastebin.mozilla.org/1637716

> Another possibility is to only show the module name in about:compartments. 
> If the primary use case is checking whether the module has been loaded, that
> would suffice.  (In fact, about:compartments is much easier than
> about:memory for that.)  And then about:memory wouldn't be affected.


While merging the other, unnamed compartments together is most appropriate, I do think that having the named one show up separately is the most useful way, also in about:memory. While a major use case is simply checking if the module is loaded, a secondary use case is observing the actual measure to debug memory "leaks" (like ever-growing data-caches). Hence I don't think that merging away that info is a good idea.
However, other ways to present it might work better in about:memory, like using the expando stuff to auto-hide the branches by default even in ?verbose.
But that this discussion would be more appropriate for another, follow-up bug IMHO, as this one has nothing to do with the actual about:memory implementation. Other tools may use the same information this bug adds but present it in a more meaningful manner, like the extension that generated combined reports per sdk-addon (which is only possible because Sandboxes profile the location already)
> >  Bug 752381
> > deliberately merged all the System Principal compartments after CPG landed,
> > because there were 100s and it was totally distorting the measurements.
> 
> That was about:memory?verbose, of course, not about:memory, just if anybody
> else wonders.

Nope, the problem exists in both versions -- very similar text is generated in both verbose and non-verbose modes.  In non-verbose mode most of it is collapsed by defeault, but you can click to expand.


> While merging the other, unnamed compartments together is most appropriate,
> I do think that having the named one show up separately is the most useful
> way, also in about:memory.

Sure, all else being equal, more information is better.  But if the extra information perturbs what is being measured excessively, then the measurement becomes useless.  That's what bug 752381 fixed.
Nils told me privately that this patch increases the number of entries in the "explicit" tree of Firefox (with no add-ons) at start-up from 195 to 1343.  IMO, this will create too much perturbation in about:memory, unfortunately.

This is a nice patch, but we need to get the amount of memory consumed by about:memory down for it to land :(
Comment on attachment 623591 [details] [diff] [review]
patch v1, add the location

r- based on comment 7, then. Other than that, the patch looks good, though.
Attachment #623591 - Flags: review?(mrbkap) → review-
Depends on: 755583
Whiteboard: [MemShrink] → [MemShrink:P1]
From the pastebin in comment 5:


System Compartments
[System Principal] [66]
[System Principal], resource:///modules/DownloadsCommon.jsm
[System Principal], resource:///modules/NetworkPrioritizer.jsm
[System Principal], resource:///modules/PageThumbs.jsm
[System Principal], resource:///modules/PlacesUIUtils.jsm
[System Principal], resource:///modules/TelemetryTimestamps.jsm
[System Principal], resource:///modules/WebappsInstaller.jsm
[System Principal], resource:///modules/devtools/scratchpad-manager.jsm
[System Principal], resource:///modules/distribution.js
[System Principal], resource:///modules/sessionstore/XPathGenerator.jsm
[System Principal], resource:///modules/webappsUI.jsm
[System Principal], resource://gre/modules/AddonLogging.jsm
[System Principal], resource://gre/modules/AddonManager.jsm
[System Principal], resource://gre/modules/FileUtils.jsm
[System Principal], resource://gre/modules/InlineSpellChecker.jsm
[System Principal], resource://gre/modules/LightweightThemeConsumer.jsm
[System Principal], resource://gre/modules/LightweightThemeManager.jsm
[System Principal], resource://gre/modules/NetUtil.jsm
[System Principal], resource://gre/modules/PlacesUtils.jsm
[System Principal], resource://gre/modules/PluginProvider.jsm
[System Principal], resource://gre/modules/Services.jsm
[System Principal], resource://gre/modules/TelemetryStopwatch.jsm
[System Principal], resource://gre/modules/Webapps.jsm
[System Principal], resource://gre/modules/WindowDraggingUtils.jsm
[System Principal], resource://gre/modules/XPCOMUtils.jsm
[System Principal], resource://gre/modules/XPIProvider.jsm
[System Principal], resource://gre/modules/ctypes.jsm
[System Principal], resource://gre/modules/debug.js
[System Principal], resource://services-common/async.js
[System Principal], resource://services-common/log4moz.js
[System Principal], resource://services-common/observers.js
[System Principal], resource://services-common/preferences.js
[System Principal], resource://services-common/stringbundle.js
[System Principal], resource://services-common/utils.js
[System Principal], resource://services-crypto/utils.js
[System Principal], resource://services-sync/constants.js
[System Principal], resource://services-sync/identity.js
[System Principal], resource://services-sync/keys.js
[System Principal], resource://services-sync/main.js
[System Principal], resource://services-sync/status.js
[System Principal], resource://services-sync/util.js
atoms
moz-nullprincipal:{3c9aef62-6f96-a046-a956-6ae7b05bc577}


I'd love to know what those other 66 System Principal compartments are.  Presumably for each kind of System Principal compartment, there's a place in the code where it's created where we could set the location, just as this patch does for JSMs?
Since you asked in comment #9:
I did some some gdb'ing to find out what code paths create compartments, and added corresponding code.
The gdb'ing is detailed in https://gist.github.com/2711823 Use a debug build

The patch isn't completely polished and my moz-C++ skills are a bit rusty, but it is enough for a PoC for now.

There are two code paths I did not work on. The first is the "main" compartment (looks like?) and the second is IPC frame scripts (looks like?).

The followin gist shows the resulting about:compartments?verbose:
https://gist.github.com/2711868
Attachment #624412 - Flags: feedback?(n.nethercote)
The two remaining code paths are:
#1 CreateNewCompartment
#2 xpc_CreateGlobalObject
#3 XPCJSContextStack::GetSafeJSContext
#4 nsXPConnect::GetSafeJSContext
#5 nsScriptSecurityManager::GetSafeJSContext
#6 nsScriptSecurityManager::Init
#7 nsScriptSecurityManager::GetScriptSecurityManager
#8 nsContentUtils::Init
#9 nsLayoutStatics::Initialize
#10 Initialize
#11 nsComponentManagerImpl::KnownModule::Load

#1 CreateNewCompartment
#2 xpc_CreateGlobalObject
#3 XPCWrappedNative::WrapNewGlobal
#4 nsXPConnect::InitClassesWithNewWrappedGlobal
#5 nsFrameScriptExecutor::InitTabChildGlobalInternal
#6 nsInProcessTabChildGlobal::InitTabChildGlobal
#7 nsInProcessTabChildGlobal::Init
#8 nsInProcessTabChildGlobal::LoadFrameScript
#9 nsAsyncScriptLoad::Run
#10 nsContentUtils::RemoveScriptBlocker
#11 PresShell::DidCauseReflow
Comment on attachment 624412 [details] [diff] [review]
patch, add location information for js modules, js components, xul scripts, xul docs, xbl docs

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

Including xpcprivate.h is no good. Add something like xpc::SetLocationForGlobal(JSObject*, const nsACString&) to xpcpublic.h and see how far that gets you.

::: js/xpconnect/loader/mozJSComponentLoader.cpp
@@ +610,4 @@
>      // Cache this module for later
>      if (!mModules.Put(spec, entry))
>          return NULL;
> +    else {

Else after return is forbidden
Attachment #624412 - Flags: feedback-
(In reply to Ms2ger from comment #12)

> Including xpcprivate.h is no good. Add something like
> xpc::SetLocationForGlobal(JSObject*, const nsACString&) to xpcpublic.h and
> see how far that gets you.

Alright then. Created a xpc::SetLocationForGlobal() and put that into xpcpublic.h/nsXPConnect.cpp

> Else after return is forbidden

Copy paste error. Meant to clean that up, but forgot.
Attachment #624412 - Attachment is obsolete: true
Attachment #624412 - Flags: feedback?(n.nethercote)
Attachment #624477 - Flags: feedback?(Ms2ger)
Comment on attachment 624477 [details] [diff] [review]
patch take 2, add location information for js modules, js components, xul scripts, xul docs, xbl docs

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

This looks better to me, thanks!

::: content/xbl/src/nsXBLPrototypeBinding.cpp
@@ +1708,5 @@
> +  // Set the location information for the new global, so that
> +  // tools like about:memory may use that information
> +  nsCOMPtr<nsIURI> docURI;
> +  mBindingURI->Clone(getter_AddRefs(docURI));
> +  docURI->SetRef(EmptyCString());

Probably need to check the result for these

@@ +1711,5 @@
> +  mBindingURI->Clone(getter_AddRefs(docURI));
> +  docURI->SetRef(EmptyCString());
> +  nsCString spec;
> +  nsresult rvspec = docURI->GetSpec(spec);
> +  NS_ABORT_IF_FALSE(NS_SUCCEEDED(rvspec), "binding uri has no spec!");

Not clear to me why this is necessarily true

::: content/xul/content/src/Makefile.in
@@ +75,4 @@
>  	-I$(srcdir)/../../../events/src \
>  	-I$(srcdir)/../../../xbl/src \
>  	-I$(topsrcdir)/xpcom/ds \
> +	-I$(topsrcdir)/js/xpconnect/src \

You should not need this now

::: js/xpconnect/src/nsXPConnect.cpp
@@ +2647,4 @@
>  }
>  #endif
>  
> +bool SetLocationForGlobal(JSObject *global, const nsACString& location) {

Add some newlines:

bool
SetLocationForGlobal(JSObject *global, const nsACString& location)
{

@@ +2651,5 @@
> +    if (!global || location.IsEmpty())
> +      return false;
> +
> +    JSCompartment *compartment = js::GetObjectCompartment(global);
> +    NS_ABORT_IF_FALSE(compartment, "No compartment for global");

MOZ_ASSERT, if this is indeed assertable. (I assume so)
Attachment #624477 - Flags: feedback?(Ms2ger) → feedback?(n.nethercote)
Comment on attachment 624477 [details] [diff] [review]
patch take 2, add location information for js modules, js components, xul scripts, xul docs, xbl docs

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

I'm not familiar enough with XPConnect to really comment on the specific code changes in this patch, but I'll give you a resounding f+ for the output :)

::: content/xbl/src/nsXBLPrototypeBinding.cpp
@@ +21,4 @@
>   *
>   * Contributor(s):
>   *   Original Author: David W. Hyatt (hyatt@netscape.com)
> + *   Nils Maier <maierman@web.de>

The conventional wisdom is to not add your name to files in this manner;  if everyone who modified a file added their name, every file would have dozens of names.  In fact, I believe we're moving away from names in files at all.
Attachment #624477 - Flags: feedback?(n.nethercote) → feedback+
> In fact, I believe we're moving away from names in files at all.

Yes, the licenses are getting changed to MPL2, which doesn't have the name blocks, on Monday.
Comment on attachment 624477 [details] [diff] [review]
patch take 2, add location information for js modules, js components, xul scripts, xul docs, xbl docs

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

::: content/xul/content/src/nsXULElement.cpp
@@ +2994,5 @@
> +
> +    // Set the location information for the new global, so that
> +    // tools like about:memory may use that information
> +    nsCString spec;
> +    nsresult rvspec = aDocumentURI->GetSpec(spec);

I get crashes here -- aDocumentURI can be nsnull.  It's weird, after recompiling it works once, and then I get the crash.  After 3 crashes, it tries to restart in safe mode, and I choose "quit", and then it works once again and the cycle repeats.
> I'd love to know what those other 66 System Principal compartments are.

Sandboxes, maybe?

I agree we should be able to annotate them, whatever they are.
(In reply to Boris Zbarsky (:bz) from comment #18)
> > I'd love to know what those other 66 System Principal compartments are.
> 
> Sandboxes, maybe?
> 
> I agree we should be able to annotate them, whatever they are.

Nils already did that in his second patch :)  See https://gist.github.com/2711868 for example output, there is only a single undistinguished System Principal compartment, probably covered by one of the stack traces in comment 11.
Ah, ok!  I should really read the whole bug, eh?  ;)

Looking at the patch, I don't understand why it's changing nsXBLPrototypeBinding::Write.  Or nsXULPrototypeScript::Deserialize for that matter.  Would it not be better to change the places that create the globals instead?

For that matter, the nsGlobalWindow change is not making much sense to me...
I would guess that that last JS compartment is the compartment for the safe JSContext, especially if it's the first compartment created.
The improvements
No longer blocks: 756261
No longer depends on: 755583
(Not sure what happened in comment 22...)

Bug 755583 improved things enough that I'm happy for this to go forward now.  Sounds like comment 20 needs to be addressed, though.
Depends on: 755583
(In reply to Nicholas Nethercote [:njn] from comment #23)
> (Not sure what happened in comment 22...)
> 
> Bug 755583 improved things enough that I'm happy for this to go forward now.
> Sounds like comment 20 needs to be addressed, though.

What's the (rough) allocation overhead of about:memory with this patch and bug 755583 compared to without this patch and without bug 755583?
> What's the (rough) allocation overhead of about:memory with this patch and
> bug 755583 compared to without this patch and without bug 755583?

The number of lines in the "explicit" tree is an approximate measure.  Before, on start-up with no-addons installed, it was something like 300, now it's something like 1200.  If you have jetpack add-ons installed the increase will be larger, because there are more System Principal compartments.

But I've made about:memory a bit (10--15%?) more efficient, and I have a few more ideas on that front that I'll try today.
(In reply to Boris Zbarsky (:bz) from comment #20)

> Looking at the patch, I don't understand why it's changing
> nsXBLPrototypeBinding::Write.  Or nsXULPrototypeScript::Deserialize for that
> matter.  Would it not be better to change the places that create the globals
> instead?
> 
> For that matter, the nsGlobalWindow change is not making much sense to me...

Well, I first only wanted js-modules info.
The rest is more of a PoC.

Regarding "places that create the globals"... You may consider nsXBLPrototypeBinding::Write and the others as such places, as they have direct paths into JS_NewCompartmentAndGlobalObject. See comment #10. 
Indeed, it might be better to change the functions that actually create the compartments, namely xpc_CreateGlobalObject or JS_NewCompartmentAndGlobalObject, but I didn't want to change countless function signatures to produce something that wouldn't make it into the tree as per comment #8 ;)

Please advise?

(In reply to Nicholas Nethercote [:njn] from comment #25)
> If you have jetpack add-ons installed the
> increase will be larger, because there are more System Principal
> compartments.

Actually, jetpack compartments and bootstrap.js compartments, being sandboxes with a sandboxName, already have show up as different compartments without this patch since a few Firefox releases. This was implemented in bug 673331.

(In reply to Nicholas Nethercote [:njn] from comment #17)
> ::: content/xul/content/src/nsXULElement.cpp
> @@ +2994,5 @@
> > +
> > +    // Set the location information for the new global, so that
> > +    // tools like about:memory may use that information
> > +    nsCString spec;
> > +    nsresult rvspec = aDocumentURI->GetSpec(spec);
> 
> I get crashes here -- aDocumentURI can be nsnull.  It's weird, after
> recompiling it works once, and then I get the crash.  After 3 crashes, it
> tries to restart in safe mode, and I choose "quit", and then it works once
> again and the cycle repeats.

This shouldn't actually happen from what I understand, and did not happen for me... 
I'll look into it. Any additional STR?
> as they have direct paths into JS_NewCompartmentAndGlobalObject

That's pretty darned surprising for nsXBLPrototypeBinding::Write.  What's the stack?
(In reply to Boris Zbarsky (:bz) from comment #27)
> > as they have direct paths into JS_NewCompartmentAndGlobalObject
> 
> That's pretty darned surprising for nsXBLPrototypeBinding::Write.  What's
> the stack?

OK, XBLProtoypeBinding has a few XBL-related stops in between that might indeed be more appropriate.
nsGlobalWindow::SetNewDocument OTOH, calls directly into nsJSContext::CreateNativeGlobalForInner, for example.

Processed log I just took... https://gist.github.com/2763057
I accidentally discarded the original log, so this one was taken just now.
> Processed log I just took... https://gist.github.com/2763057

Thanks.

So yes, putting this stuff in nsXBLDocGlobalObject would make more sense; nothing guarantees that the first call to EnsureScriptEnvironment when there isn't a script env yet will come via nsXBLPrototypeBinding::Write.

> nsGlobalWindow::SetNewDocument OTOH, calls directly into
> nsJSContext::CreateNativeGlobalForInner

Sure; I think it would make more sense to pass the relevant info (e.g. the nsIURI) to that method.  I'm quite sure now what really didn't make sense to me about this code before...
Summary: Add identifying information to js code module system compartments → Add identifying information to all system compartments
New patch, with the remarks and suggestions from previous comments addressed.

A resulting about:memory can be seen here:
https://gist.github.com/2764014
Attachment #623591 - Attachment is obsolete: true
Attachment #624477 - Attachment is obsolete: true
Attachment #625721 - Flags: review?(bzbarsky)
Comment on attachment 625721 [details] [diff] [review]
Patch v1, add location information for jsm, jscomponents, xul, xbl, inner windows

So here's a question.  Can we have the CompartmentPrivate hold on to the URI in cases when we actually have a URI?  GetSpec can be pretty darned expensive in some cases...
As per comment 31, the URI will now be stored instead of an early GetSpec call. Subsequently GetSpec will only be called on that stored URI when the location is actually requested, e.g. by the JS MultiReporter/about:memory. The result of GetSpec is stored, so that there will be one GetSpec call at maximum. i.e. the location is now lazily initialized.

Now you got two variants to choose from ;)
Attachment #625793 - Flags: review?(bzbarsky)
> > > +    nsresult rvspec = aDocumentURI->GetSpec(spec);
> > 
> > I get crashes here -- aDocumentURI can be nsnull.
> 
> This shouldn't actually happen from what I understand, and did not happen
> for me... 
> I'll look into it. Any additional STR?

DeserializeOutOfLine() calls Deserialize() with the 3rd arg set to nsnull.  See http://mxr.mozilla.org/mozilla-central/source/content/xul/content/src/nsXULElement.cpp#3002

It happened for me very reliably, on multiple profiles, but the DeserializeOutOfLine() call should be enough for you to go on :)
(In reply to Nicholas Nethercote [:njn] from comment #34)
> 
> DeserializeOutOfLine() calls Deserialize() with the 3rd arg set to nsnull. 
> See
> http://mxr.mozilla.org/mozilla-central/source/content/xul/content/src/
> nsXULElement.cpp#3002
> 
> It happened for me very reliably, on multiple profiles, but the
> DeserializeOutOfLine() call should be enough for you to go on :)

The ::Deserialize code gone in the last two patches, i.e. this is not an issue any longer. ;)
Should have canceled my STR request... Sorry.
Comment on attachment 625793 [details] [diff] [review]
Patch v2, add location information for jsm, jscomponents, xul, xbl, inner windows

>+++ b/content/xbl/src/nsXBLDocumentInfo.cpp
>@@ -281,6 +281,18 @@

Please add -p to your diff options?  Or add "showfunc = true" to your .hgrc [diff] section.  Would make this way simpler to review.

>+    nsIURI *ownerURI = mGlobalObjectOwner->DocumentURI();
>+    if (ownerURI)

DocumentURI() never returns null.  So no need to branch here.

>+  else
>+    xpc::SetLocationForGlobal(mJSObject, NS_LITERAL_CSTRING("<unknown XBL document>"));

mGlobalObjectOwner is never null either, so no need for that null-check.

>+++ b/content/xul/document/src/nsXULPrototypeDocument.cpp
>+  if (mGlobalObjectOwner) {

Again, I believe this can never be null.

>+    nsIURI *ownerURI = mGlobalObjectOwner->GetURI();

Nor can the return value of GetURI() here.

>+++ b/dom/base/nsGlobalWindow.cpp
>+      if (aDocument)

aDocument is never null here.

>+++ b/dom/base/nsIScriptContext.h

You need to change the IID.

>+++ b/dom/base/nsJSEnvironment.cpp
>+  // Set the location information for the new global, so that tools like
>+  // about:memory may use that information
>+  if (aNativeGlobal && *aNativeGlobal) {

aNativeGlobal better never be null here.  Neither will *aNativeGlobal; you can assert that if desired.

>+    if (aURI)

aURI won't be null either.

>+++ b/js/xpconnect/loader/mozJSComponentLoader.cpp
>@@ -1220,6 +1224,11 @@
>+    // Set the location information for the new global, so that tools like
>+    // about:memory may use that information
>+    if (*_retval)
>+        xpc::SetLocationForGlobal(*_retval, aLocation);

Wouldn't this make more sense right after the GlobalForLocation call, so you're not repeatedly setting the location for the same global?

Also, *_retval is guaranteed non-null here, no?

>+++ b/js/xpconnect/src/nsXPConnect.cpp
>+bool
>+SetLocationForGlobal(JSObject *global, const nsACString& location)

Every single caller ignores the (undocumented, anyway) return value.  Just make these void functions, please.

r=me with those nits picked.  Thanks!
Attachment #625793 - Flags: review?(bzbarsky) → review+
Assignee: nobody → MaierMan
Status: NEW → ASSIGNED
Comment on attachment 625793 [details] [diff] [review]
Patch v2, add location information for jsm, jscomponents, xul, xbl, inner windows

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

::: dom/base/nsJSEnvironment.cpp
@@ +2099,5 @@
> +  if (aNativeGlobal && *aNativeGlobal) {
> +    if (aURI)
> +      xpc::SetLocationForGlobal(*aNativeGlobal, aURI);
> +    else
> +      xpc::SetLocationForGlobal(*aNativeGlobal, NS_LITERAL_CSTRING("<unknown inner window>"));

Outside XPConnect, single-line ifs need to have {}s

::: js/xpconnect/src/nsXPConnect.cpp
@@ +2613,5 @@
> +bool
> +SetLocationForGlobal(JSObject *global, const nsACString& location)
> +{
> +    if (!global)
> +      return false;

4-space indentation

@@ +2629,5 @@
> +bool
> +SetLocationForGlobal(JSObject *global, nsIURI *locationURI)
> +{
> +    if (!global)
> +      return false;

id.

::: js/xpconnect/src/xpcprivate.h
@@ +4411,5 @@
> +            locationURI = nsnull;
> +        }
> +        return location;
> +    }
> +    bool SetLocation(const nsACString& aLocation) {

Empty lines between those

::: js/xpconnect/src/xpcpublic.h
@@ +229,5 @@
>  void DumpJSHeap(FILE* file);
>  #endif
>  
> +bool SetLocationForGlobal(JSObject* global, const nsACString& location);
> +bool SetLocationForGlobal(JSObject* global, nsIURI *locationURI);

* on the same side for both arguments, please.
"Nits" picked.
And showsource within my .hgrc

However, stuff changed quite a lot, especially those removed NULL checks, so IMO another (fast) review would be appropriate.

Do I need an sr for that, btw?
Attachment #625721 - Attachment is obsolete: true
Attachment #625793 - Attachment is obsolete: true
Attachment #625721 - Flags: review?(bzbarsky)
Attachment #625988 - Flags: review?(bzbarsky)
Attachment #625988 - Flags: feedback?(Ms2ger)
Comment on attachment 625988 [details] [diff] [review]
Patch v3, add location information for jsm, jscomponents, xul, xbl, inner windows

r=me

I don't think you need sr here.
Attachment #625988 - Flags: review?(bzbarsky) → review+
Attachment #625988 - Flags: feedback?(Ms2ger) → feedback+
(In reply to Boris Zbarsky (:bz) from comment #39)
> I don't think you need sr here.

Somebody land this then, please.
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e5c6770680a
Flags: in-testsuite-
Keywords: checkin-needed
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/4e5c6770680a
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Blocks: 759783
Blocks: 760265
You need to log in before you can comment on or make changes to this bug.