822135 - Disable Profiler when there's a Private Browsing Window

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

[Benoit Girard (:BenWa)]

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.

Comment 1

[Josh Matthews [:jdm]]

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.

Comment 2

[Benoit Girard (:BenWa)]

(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.

Comment 3

[Josh Matthews [:jdm]]

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

Comment 4

[Benoit Girard (:BenWa)]

Attachment: patch

Comment 5

[Josh Matthews [:jdm]]

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 6

[Benoit Girard (:BenWa)]

Attachment: patch

Comment 7

[Josh Matthews [:jdm]]

Comment on attachment 693147 [details] [diff] [review]
patch

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

Have you tested that this does what you want?

Comment 8

[Benoit Girard (:BenWa)]

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 9

[(no longer active)]

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...

Comment 10

[Benoit Girard (:BenWa)]

Sounds good to me

https://tbpl.mozilla.org/?tree=Try&rev=f6b072f12e3a

Comment 11

[Benoit Girard (:BenWa)]

https://tbpl.mozilla.org/?tree=Try&rev=2b01135f19cc

Comment 12

[(no longer active)]

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

Comment 13

[Benoit Girard (:BenWa)]

We also need bug 795961 fixed.

Comment 14

[Benoit Girard (:BenWa)]

Attachment: patch r=josh

https://tbpl.mozilla.org/?tree=Try&rev=5e92e83904f4

Comment 15

[Benoit Girard (:BenWa)]

https://tbpl.mozilla.org/?tree=Try&rev=67dc81308619

Comment 16

[Benoit Girard (:BenWa)]

Attachment: patch r=jdm

Comment 17

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f5ec1efd39ee

Comment 18

[Josh Matthews [:jdm]]

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 19

[Benoit Girard (:BenWa)]

Attachment: fix reference

Comment 20

[(no longer active)]

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.  ;-)

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f5ec1efd39ee

Comment 22

[Benoit Girard (:BenWa)]

Opps I forgot to land this.

Comment 23

[Benoit Girard (:BenWa)]

Attachment: MozReview Request: Bug 822135 - Fix nsProfiler observer strong reference. r=ehsan

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

Comment 24

[Benoit Girard (:BenWa)]

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

Comment 25

[Benoit Girard (:BenWa)]

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

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 27

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

The main patch landed in Dec 2012, so I'm calling this bug fixed.