855866 - usePrivateBrowsing is true for non-private windows on domwindowopen and toplevel-window-ready

Status: RESOLVED FIXED

(Core :: Widget, defect)

Description

[Alexandre Poirot [:ochameau]]

In Jetpack, we used to listen to domwindowopen for new top level window creation,
but it ends up being buggy as window.usePrivateBrowsing is true on non-private windows opened from a private window.

STR:
1/ Open firefox,
2/ Open a private window,
3/ Open a non-private window from the private one,

Actual:
Receive domwindowopen observer notification, with a window.usePrivateBrowsing being true.

Excepted:
Ideally, domwindowopen will only fire when the window is preliminaty initialized and do not reflect ambigiuous states.
So that window.usePrivateBrowsing should be false.


Such behavior happens because private browsing state is set after these two notification are fired.
The state is set at the end of nsWindowWatcher::OpenWindowInternal 
  http://mxr.mozilla.org/mozilla-central/source/embedding/components/windowwatcher/src/nsWindowWatcher.cpp#904
Whereas `domwindowopened` opened is fired when we call windowCreator2->CreateChromeWindow2 at the beginning of this OpenWindowInternal method:
  http://mxr.mozilla.org/mozilla-central/source/embedding/components/windowwatcher/src/nsWindowWatcher.cpp#739
And still right after the `toplevel-window-ready` event:
  http://mxr.mozilla.org/mozilla-central/source/embedding/components/windowwatcher/src/nsWindowWatcher.cpp#863

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: Bug 855866: Fire toplevel-window-ready after usePrivateBrowsing is initialized.

Comment 2

[Alexandre Poirot [:ochameau]]

Here is a patch to fire toplevel-window-ready after usePrivateBrowsing is initialized.
But ideally the private state would be correctly initialized before we even fire domwindowopened. Or fire both of these events later... ?

Comment 3

[Erik Vold [:erikvold] (please needinfo? me)]

Can we make this critical?

Comment 4

[(no longer active)]

Comment on attachment 730907 [details] [diff] [review]
Bug 855866: Fire toplevel-window-ready after usePrivateBrowsing is initialized.

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

Hmm, this patch causes the notification to not be dispatched if |!(uriToLoad && aNavigate)|.  I think you should move this block down to after |if (isNewToplevelWindow)|.  You want to get bz to review this patch though.

Comment 5

[(no longer active)]

Is this still an issue?

Comment 6

[Alexandre Poirot [:ochameau]]

Attachment: Fire toplevel-window-ready after usePrivateBrowsing is initialized, but still before the document starts loading.

Comment 7

[Alexandre Poirot [:ochameau]]

Comment on attachment 737950 [details] [diff] [review]
Fire toplevel-window-ready after usePrivateBrowsing is initialized, but still before the document starts loading.

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #4)
> Comment on attachment 730907 [details] [diff] [review]
> Bug 855866: Fire toplevel-window-ready after usePrivateBrowsing is
> initialized.
> 
> Review of attachment 730907 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hmm, this patch causes the notification to not be dispatched if |!(uriToLoad
> && aNavigate)|.  I think you should move this block down to after |if
> (isNewToplevelWindow)|.  You want to get bz to review this patch though.

Oh, good catch!
I tweaked the patch accordingly, but didn't moved it in |if (isNewToplevelWindow)| as it would fired the event after the document starts loading.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 737950 [details] [diff] [review]
Fire toplevel-window-ready after usePrivateBrowsing is initialized, but still before the document starts loading.

r=me

Comment 9

[(no longer active)]

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

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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