Open Bug 822135 Opened 9 years ago Updated 5 years ago

Disable Profiler when there's a Private Browsing Window

Categories

(Core :: Gecko Profiler, defect)

x86
macOS
defect
Not set
normal

Tracking

()

REOPENED
mozilla21

People

(Reporter: BenWa, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 3 obsolete files)

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.
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → bgirard
Status: NEW → ASSIGNED
Attachment #693046 - Flags: review?(josh)
Attachment #693046 - Flags: review?(ehsan)
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
Attachment #693046 - Flags: review?(josh)
Attached patch patch (obsolete) — Splinter Review
Attachment #693046 - Attachment is obsolete: true
Attachment #693046 - Flags: review?(ehsan)
Attachment #693147 - Flags: review?(josh)
Comment on attachment 693147 [details] [diff] [review]
patch

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

Have you tested that this does what you want?
Attachment #693147 - Flags: review?(josh) → review+
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.
Attachment #693147 - Flags: review?(ehsan)
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...
Attachment #693147 - Flags: review?(ehsan)
Sounds good to me

https://tbpl.mozilla.org/?tree=Try&rev=f6b072f12e3a
Flags: needinfo?(bgirard)
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.
Flags: needinfo?(bgirard)
Attached patch patch r=josh (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=5e92e83904f4
Attachment #693147 - Attachment is obsolete: true
Attachment #700724 - Flags: review+
Attached patch patch r=jdmSplinter Review
Attachment #700724 - Attachment is obsolete: true
Attachment #701119 - Flags: review+
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.
Attached patch fix referenceSplinter Review
Attachment #701172 - Flags: review?(ehsan)
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.  ;-)
Attachment #701172 - Flags: review?(ehsan) → review+
https://hg.mozilla.org/mozilla-central/rev/f5ec1efd39ee
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Opps I forgot to land this.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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
Flags: needinfo?(bgirard)
Flags: needinfo?(bgirard)
Assignee: bgirard → nobody
Blocks: 1329163
You need to log in before you can comment on or make changes to this bug.