690952 - Remove the crazy navigator preservation behavior

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

We should stop preserving window.navigator when we jump from one page to another same-origin page. Sicking tested and it looks like we're the only browser that does this, so let's quit.

Comment 1

[Johnny Stenback (:jst)]

Agreed! But I thought I filed a bug on this already... so this might be a dupe...

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

For what it's worth, i wasn't able to replicate this behavior in any other browser.

There's of course still a chance that some website has gecko-specific code paths that rely on this, but it still feels like the right thing to do in order to reduce differences between browsers.

Unfortunately I can't think of a way to warn about this in the console. Warning any time someone sets an expando on the navigator object seems wrong.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

I have a partial patch for this lying around. I think the only sticking point is that it isn't easy to preserve the wrapper so expandos don't disappear during GC. The last time I looked, all of the wrapper preservation stuff was based around nodes.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Wrapper preservation is easy to do these days for non-nodes. Wanna attach the patch you've got here and I'll see if I can make it work?

Comment 5

[Johnny Stenback (:jst)]

I have a patch for this, passes tests n' all. I chose not to worry about explicit wrapper preservation and instead decided to leave the code that defines window.navigator as a permanent property on the inner window, which means it'll never get collected (until we navigate away or what not).

Comment 6

[Johnny Stenback (:jst)]

Attachment: Move navigator from outer to inner window, and stop preserving across same origin navigations.

Comment 7

[Johnny Stenback (:jst)]

Oh, and ignore the XXXjst comment in nsNavigatorSH::PreCreate(), we still need it per discussion with mrbkap earlier on irc.

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 577449 [details] [diff] [review]
Move navigator from outer to inner window, and stop preserving across same origin navigations.

>--- a/dom/base/Navigator.h
>+++ b/dom/base/Navigator.h

>+#include "nsPIDOMWindow.h"

Do you need this include? On first sight, I think a forward declaration would be enough.

>--- a/dom/base/nsDOMClassInfo.cpp
>+++ b/dom/base/nsDOMClassInfo.cpp
>@@ -6828,6 +6828,8 @@ nsWindowSH::NewResolve(nsIXPConnectWrapp
>+      // Hold on to the navigator object as a global property so we
>+      // don't need to worry about loosing expando properties etc.

'losing'?

>@@ -7176,20 +7178,17 @@ nsNavigatorSH::PreCreate(nsISupports *na
>+  nsGlobalWindow *win = static_cast<nsGlobalWindow*>(nav->GetWindow());

This seems to be the only caller of GetWindow. How about returning nsIScriptGlobalObject* instead?

>--- a/dom/base/nsGlobalWindow.cpp
>+++ b/dom/base/nsGlobalWindow.cpp
>@@ -2513,8 +2430,8 @@ nsGlobalWindow::SetDocShell(nsIDocShell*
>+  NS_ASSERTION(!mNavigator, "Non-null mNavigator in outer window!");

Fatal assert? But is this still right if you move navigator to the inner window?

>--- a/dom/base/nsPluginArray.cpp
>+++ b/dom/base/nsPluginArray.cpp
> nsPluginArray::AllowPlugins()
> {
>   bool allowPlugins = false;
>-  if (mDocShell)
>-    if (NS_FAILED(mDocShell->GetAllowPlugins(&allowPlugins)))
>+  nsCOMPtr<nsIDocShell> docShell(do_QueryReferent(mDocShell));

Can't do_QR use '='?

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 577449 [details] [diff] [review]
Move navigator from outer to inner window, and stop preserving across same origin navigations.

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

::: dom/base/Navigator.cpp
@@ +538,5 @@
>                                    const nsAString& aTitle)
>  {
> +  nsCOMPtr<nsPIDOMWindow> win(do_QueryReferent(mWindow));
> +
> +  if (!win || !win->GetOuterWindow()) {

This is supposed to work even after the page has navigated, right?

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Ms2ger from comment #8)
> >--- a/dom/base/nsGlobalWindow.cpp
> >+++ b/dom/base/nsGlobalWindow.cpp
> >@@ -2513,8 +2430,8 @@ nsGlobalWindow::SetDocShell(nsIDocShell*
> >+  NS_ASSERTION(!mNavigator, "Non-null mNavigator in outer window!");
> 
> Fatal assert? But is this still right if you move navigator to the inner
> window?

Yes. SetDocShell is only called on outer windows.

Comment 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

I can't read, I thought it was asserting that the outer window did have a non-null navigator.

Comment 12

[Johnny Stenback (:jst)]

(In reply to Ms2ger from comment #8)
> >+#include "nsPIDOMWindow.h"
> 
> Do you need this include? On first sight, I think a forward declaration
> would be enough.

You're right, not needed. Removed.

> >--- a/dom/base/nsDOMClassInfo.cpp
> >+++ b/dom/base/nsDOMClassInfo.cpp
> >@@ -6828,6 +6828,8 @@ nsWindowSH::NewResolve(nsIXPConnectWrapp
> >+      // Hold on to the navigator object as a global property so we
> >+      // don't need to worry about loosing expando properties etc.
> 
> 'losing'?

Fixed.

> >@@ -7176,20 +7178,17 @@ nsNavigatorSH::PreCreate(nsISupports *na
> >+  nsGlobalWindow *win = static_cast<nsGlobalWindow*>(nav->GetWindow());
> 
> This seems to be the only caller of GetWindow. How about returning
> nsIScriptGlobalObject* instead?

We could do that, but I'd rather keep the return type relevant to the name of the getter, and having a Navigator object associated with a window rather than an nsIScriptGlobalObject makes sense to me. Non-window objects can be nsIScriptGlobalObjects too (as is the case with nsXBLDocGlobalObject), so I'd prefer to leave the API as is.

> >+  nsCOMPtr<nsIDocShell> docShell(do_QueryReferent(mDocShell));
> 
> Can't do_QR use '='?

Yup, changed (though I personally tend to write code as written above, but I can't say I feel strongly either way).

Thanks for reviewing Ms2ger!

Comment 13

[Johnny Stenback (:jst)]

(In reply to Blake Kaplan (:mrbkap) from comment #9)
> ::: dom/base/Navigator.cpp
> @@ +538,5 @@
> >                                    const nsAString& aTitle)
> >  {
> > +  nsCOMPtr<nsPIDOMWindow> win(do_QueryReferent(mWindow));
> > +
> > +  if (!win || !win->GetOuterWindow()) {
> 
> This is supposed to work even after the page has navigated, right?

Good question. I don't know that there's a clear answer here. But I decided to change this around a bit to work just like it used to, i.e. check for a non-existent docshell in all places where that used to be the check.

Thanks!

Comment 14

[Johnny Stenback (:jst)]

Attachment: Updated patch for checkin.

This should contain all fixes and be merged to tip (sms changes conflicted a bit). I also changed the Navigator destructor to call Invalidate() which basically does the same thing the destructor used to do, so no real change there.

Comment 15

[Johnny Stenback (:jst)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/11d6f47eea30

Comment 16

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/11d6f47eea30

Comment 17

[Eric Shepherd [:sheppy]]

Mentioned on Firefox 11 for developers.