Closed
Bug 754771
Opened 12 years ago
Closed 12 years ago
Add identifying information to all system compartments
Categories
(Core :: XPConnect, defect)
Core
XPConnect
Tracking
()
RESOLVED
FIXED
mozilla15
People
(Reporter: nmaier, Assigned: nmaier)
References
Details
(Whiteboard: [MemShrink:P1])
Attachments
(2 files, 5 obsolete files)
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.
Assignee | ||
Comment 1•12 years ago
|
||
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)
![]() |
||
Comment 2•12 years ago
|
||
Nils, can you show a before/after comparison of about:memory with an appropriate add-on installed?
Assignee | ||
Comment 3•12 years ago
|
||
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.
![]() |
||
Comment 4•12 years ago
|
||
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.
Assignee | ||
Comment 5•12 years ago
|
||
(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)
![]() |
||
Comment 6•12 years ago
|
||
> > 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.
![]() |
||
Comment 7•12 years ago
|
||
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 8•12 years ago
|
||
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-
![]() |
||
Updated•12 years ago
|
Whiteboard: [MemShrink] → [MemShrink:P1]
![]() |
||
Comment 9•12 years ago
|
||
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?
Assignee | ||
Comment 10•12 years ago
|
||
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)
Assignee | ||
Comment 11•12 years ago
|
||
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 12•12 years ago
|
||
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-
Assignee | ||
Comment 13•12 years ago
|
||
(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 14•12 years ago
|
||
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 15•12 years ago
|
||
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+
Comment 16•12 years ago
|
||
> 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 17•12 years ago
|
||
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.
![]() |
||
Comment 18•12 years ago
|
||
> 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.
![]() |
||
Comment 19•12 years ago
|
||
(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.
![]() |
||
Comment 20•12 years ago
|
||
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.
![]() |
||
Comment 22•12 years ago
|
||
The improvements
![]() |
||
Comment 23•12 years ago
|
||
(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
Comment 24•12 years ago
|
||
(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?
![]() |
||
Comment 25•12 years ago
|
||
> 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.
Assignee | ||
Comment 26•12 years ago
|
||
(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?
![]() |
||
Comment 27•12 years ago
|
||
> as they have direct paths into JS_NewCompartmentAndGlobalObject
That's pretty darned surprising for nsXBLPrototypeBinding::Write. What's the stack?
Assignee | ||
Comment 28•12 years ago
|
||
(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.
![]() |
||
Comment 29•12 years ago
|
||
> 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...
Assignee | ||
Updated•12 years ago
|
Summary: Add identifying information to js code module system compartments → Add identifying information to all system compartments
Assignee | ||
Comment 30•12 years ago
|
||
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 31•12 years ago
|
||
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...
Comment 32•12 years ago
|
||
(For posterity.)
Assignee | ||
Comment 33•12 years ago
|
||
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)
![]() |
||
Comment 34•12 years ago
|
||
> > > + 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 :)
Assignee | ||
Comment 35•12 years ago
|
||
(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 36•12 years ago
|
||
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+
Updated•12 years ago
|
Assignee: nobody → MaierMan
Status: NEW → ASSIGNED
Comment 37•12 years ago
|
||
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.
Assignee | ||
Comment 38•12 years ago
|
||
"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 39•12 years ago
|
||
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+
Updated•12 years ago
|
Attachment #625988 -
Flags: feedback?(Ms2ger) → feedback+
Assignee | ||
Comment 40•12 years ago
|
||
(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
Comment 42•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4e5c6770680a
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•