The profiler isn't tab aware and it's far from a simple job. For now I discussed this with Ehsan that opening the first private window should force disable the profiler and that closing the last private window should restore the state. Ehsan/Josh what are the right events to listen for? Since the profiler devtools panel is now landed I think this should live in the profiler xpcom module so it can be shared between the two.

There's no event for the first opening of a private window right now. It would be nice to avoid requiring one as well. As for closing, that's last-pb-context-exited.

(In reply to Josh Matthews [:jdm] from comment #1) > There's no event for the first opening of a private window right now. It > would be nice to avoid requiring one as well. As for closing, that's > last-pb-context-exited. What about knowing when <i>a</i> private window is open? Otherwise we could have a PB poke the profiler specifically if we don't want to introduce events for others to misuse.

You could presumably just iterate through the open windows and check PrivateBrowsingUtils.isWindowPrivate to get that information.

Comment on attachment 693046 [details] [diff] [review] patch Review of attachment 693046 [details] [diff] [review]: ----------------------------------------------------------------- ::: tools/profiler/nsIProfiler.idl @@ +5,5 @@ > > #include "nsISupports.idl" > > [scriptable, uuid(c94651cf-082f-45e2-9481-33a1a700b46a)] > +interface nsIProfiler : nsISupports Revert. ::: tools/profiler/nsProfiler.cpp @@ +30,5 @@ > { > + nsCOMPtr<nsIObserverService> observerService = mozilla::services::GetObserverService(); > + if (observerService) { > + observerService->AddObserver(this, "chrome-document-global-created", false); > + observerService->AddObserver(this, "last-pb-context-exited", false); Doing this in the constructor is dangerous, since the refcount is 0. Move this to an Init function. @@ +43,5 @@ > + if (strcmp(aTopic, "chrome-document-global-created") == 0) { > + nsCOMPtr<nsIInterfaceRequestor> requestor = do_QueryInterface(aSubject); > + nsCOMPtr<nsIWebNavigation> parentWebNav = do_GetInterface(requestor); > + nsCOMPtr<nsILoadContext> loadContext = do_QueryInterface(parentWebNav); > + if (loadContext->UsePrivateBrowsing() && !mLockedForPrivateBrowsing) { Definitely need a null check here; the do_Foo ones are null safe though. @@ +59,5 @@ > nsProfiler::StartProfiler(uint32_t aEntries, uint32_t aInterval, > const char** aFeatures, uint32_t aFeatureCount) > { > + if (mLockedForPrivateBrowsing) { > + // TODO How do we return an warning? What kind of warning are you looking for? NS_ERROR_NOT_AVAILABLE will translate to a JS exception if this is called through XPConnect. ::: tools/profiler/nsProfiler.h @@ +20,5 @@ > NS_DECL_NSIPROFILER > + > + NS_IMETHODIMP Observe(nsISupports *aSubject, > + const char *aTopic, > + const PRUnichar *aData); NS_DECL_NSIOBSERVER

Comment on attachment 693147 [details] [diff] [review] patch Review of attachment 693147 [details] [diff] [review]: ----------------------------------------------------------------- Have you tested that this does what you want?

Yes, it make it impossible to use the profiler from the profiler module. However it's still possible to control the profiler from sampler.h if the platform code really wanted to profile.

Comment on attachment 693147 [details] [diff] [review] patch I think Josh's review is enough here. If you really want my review on this, please re-request but I won't get to it until Thursday...

Sounds good to me https://tbpl.mozilla.org/?tree=Try&rev=f6b072f12e3a

FWIW "chrome-document-global-created" doesn't fire for some of the windows, so this patch will not always do the right thing.

We also need bug 795961 fixed.

Comment on attachment 701119 [details] [diff] [review] patch r=jdm >+nsProfiler::~nsProfiler() >+{ >+ nsCOMPtr<nsIObserverService> observerService = >mozilla::services::GetObserverService(); >+ if (observerService) { >+ observerService->RemoveObserver(this, "chrome-document-global-created"); >+ observerService->RemoveObserver(this, "last-pb-context-exited"); >+ } >+} This won't work quite right. AddObserver is taking a strong reference to nsProfiler, so it will leak. You should probably just make it inherit from nsSupportsWeakReference, QI to nsISupportsWeakReference, and pass true to AddObserver to make it take a weak reference.

Comment on attachment 701172 [details] [diff] [review] fix reference Review of attachment 701172 [details] [diff] [review]: ----------------------------------------------------------------- ::: tools/profiler/nsProfiler.h @@ +12,3 @@ > #include "mozilla/Attributes.h" > > +class nsProfiler MOZ_FINAL : public nsIProfiler, public nsIObserver, public nsSupportsWeakReference Nit: use the Enter key. ;-)

Opps I forgot to land this.

Bug 822135 - Fix nsProfiler observer strong reference. r=ehsan

Comment on attachment 8668584 [details] MozReview Request: Bug 822135 - Fix nsProfiler observer strong reference. r=ehsan Bug 822135 - Fix nsProfiler observer strong reference. r=ehsan

https://hg.mozilla.org/integration/mozilla-inbound/rev/8f489e406701f08833fae12f66ab22f5a0d0315e Bug 822135 - Fix nsProfiler observer strong reference. r=ehsan

Backed out for mochitest-dt failures like https://treeherder.mozilla.org/logviewer.html#?job_id=15123310&repo=mozilla-inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/fc03c84dfccb