Last Comment Bug 690952 - Remove the crazy navigator preservation behavior
: Remove the crazy navigator preservation behavior
Status: RESOLVED FIXED
: addon-compat, dev-doc-complete
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla11
Assigned To: Johnny Stenback (:jst, jst@mozilla.com)
:
Mentors:
: 553559 (view as bug list)
Depends on: 732877
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-30 15:52 PDT by Ben Turner (not reading bugmail, use the needinfo flag!)
Modified: 2013-04-04 13:53 PDT (History)
15 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Move navigator from outer to inner window, and stop preserving across same origin navigations. (24.25 KB, patch)
2011-11-28 17:56 PST, Johnny Stenback (:jst, jst@mozilla.com)
mrbkap: review+
Details | Diff | Splinter Review
Updated patch for checkin. (26.08 KB, patch)
2011-12-01 00:32 PST, Johnny Stenback (:jst, jst@mozilla.com)
no flags Details | Diff | Splinter Review

Description Ben Turner (not reading bugmail, use the needinfo flag!) 2011-09-30 15:52:32 PDT
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, jst@mozilla.com) 2011-09-30 16:01:26 PDT
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 2011-09-30 16:18:31 PDT
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) 2011-09-30 16:53:54 PDT
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 2011-09-30 17:04:02 PDT
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, jst@mozilla.com) 2011-11-28 17:55:55 PST
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, jst@mozilla.com) 2011-11-28 17:56:58 PST
Created attachment 577449 [details] [diff] [review]
Move navigator from outer to inner window, and stop preserving across same origin navigations.
Comment 7 Johnny Stenback (:jst, jst@mozilla.com) 2011-11-28 17:58:40 PST
Oh, and ignore the XXXjst comment in nsNavigatorSH::PreCreate(), we still need it per discussion with mrbkap earlier on irc.
Comment 8 :Ms2ger (⌚ UTC+1/+2) 2011-11-29 00:38:51 PST
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) 2011-11-30 06:54:25 PST
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) 2011-11-30 06:55:51 PST
(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 (⌚ UTC+1/+2) 2011-11-30 07:04:13 PST
I can't read, I thought it was asserting that the outer window did have a non-null navigator.
Comment 12 Johnny Stenback (:jst, jst@mozilla.com) 2011-11-30 23:29:36 PST
(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, jst@mozilla.com) 2011-11-30 23:31:39 PST
(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, jst@mozilla.com) 2011-12-01 00:32:18 PST
Created attachment 578198 [details] [diff] [review]
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, jst@mozilla.com) 2011-12-01 00:37:39 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/11d6f47eea30
Comment 16 Matt Brubeck (:mbrubeck) 2011-12-01 11:23:34 PST
https://hg.mozilla.org/mozilla-central/rev/11d6f47eea30
Comment 17 Eric Shepherd [:sheppy] 2012-03-24 07:36:40 PDT
Mentioned on Firefox 11 for developers.
Comment 18 Blake Kaplan (:mrbkap) 2013-02-07 03:55:30 PST
*** Bug 553559 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.