163645 - User defined properties of Javascript navigator object are not remembered when a new page is loaded.

Status: RESOLVED FIXED

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

Description

[Aniruddha Apte]

User-Agent:       Mozilla/5.0 (Windows; U; WinNT4.0; en-US; rv:1.1a) Gecko/20020611
Build Identifier: Mozilla/5.0 (Windows; U; WinNT4.0; en-US; rv:1.1b) Gecko/20020818

I am trying to create user defined properties in the navigator object such as
 navigator.id = 1122334455. This property should be retained when I load a new
page and should be accessible from it. Trying to access the property from the
newly loaded page returns undefined. 

This works on 1.1a on Windows but not on 1.1b and the nightly of August18 2002.
Testing on Warpzilla (Mozilla for OS/2) looks like this...
1.1b - fails
1.1a - works 
1.0 - works
rc3 - works
rc2 - fails
rc1 - fails

This behavior works correctly on IE also.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Aniruddha Apte]

Attachment: Testcase file 1. Download both the testcase files to the same directory and then load file 1.

Comment 2

[Aniruddha Apte]

Attachment: Testcase file 2 - Open this attachment after the first one has been loaded.

Comment 3

[Aniruddha Apte]

HOW TO RECREATE: Load the 1st attachment
http://bugzilla.mozilla.org/attachment.cgi?id=96013&action=view
This creates a navigator object property, navigator.id and sets it to 1122334455

Now, in the same window load the second attachment
http://bugzilla.mozilla.org/attachment.cgi?id=96014&action=view
Clicking on the button shows an alert with the value of navigator.id
If bug exists, value of navigator.id will be undefined, else, it should say
1122334455.

Comment 4

[Peter Lubczynski]

Confirming. This appears to be a regression from bug 150087. Commenting out the
NS_RELEASE(mNavigator) in nsGlobalWindow.cpp makes the testcase work.

Comment 5

[Johnny Stenback (:jst)]

Mass-reassigning bugs to [e-mail]

Comment 6

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Seems like dataloss to me, adding keywords and upping severity. We should get to
this ASAP.

Comment 7

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Probable fix

Rather than delete the whole navigator object, just delete what we want to be
recreated.  This works against the testcase provided, and bug 150087 doesn't
seem to work for me with or without the patch...

Comment 8

[Samir Gehani]

adt: nsbeta1-

Comment 9

[Mike Kaply [:mkaply]]

This is a big one for one of our customers. Very small change...

Comment 10

[Heikki Toivonen (remove -bugzilla when emailing directly)]

I believe jst had issues with the patch, and it seems like fixing this bug would
cause a security hole as well, so the probable solution seems to be wontfix at
this point... 

Comment 11

[Mike Kaply [:mkaply]]

Can you be more specific? This is a pretty big regression for us, so we took
this fix and put it in our IBM Web Browser.

We have a huge customer that depends on this behavior...

Comment 12

[Christopher Aillon (sabbatical, not receiving bugmail)]

From my conversation with jst, this won't work all of the time.  If I remember
correctly JS will cause a GC to happen after like 2-3 seconds or something, and
the fix won't work after that.  I don't remember discussing a security issue
with him.

Comment 13

[Johnny Stenback (:jst)]

The problem with the patch is that the JS wrapper for the navigator object is
unrooted when we go from page to page, and if no script in the new page grabs a
reference to the navigator object before we run GC (~2s after we leave a page)
the JS wrapper will go away.

Other than that, there's more problems here too. This patch, even if the above
issue was fixed, only fixes part of the original problem. Real issue here is
that NS4x had only one navigator object and all windows had access to that. That
introduces security problems (such as bug 141969), and I don't think doing that
is a good idea (that behaviour was never intended in NS4x, to my knowledge).

Now IE doesn't behave exactly like that, IE does what this patch tried to do,
and while that's less insecure than what NS4x does, I can't say I like that very
much either. However, if that behaviour is absolutely needed, this patch, with
the above issue fixed, would make us behave like IE does.

Mike, is IE's behaviour what you're looking for?

Comment 14

[Mike Kaply [:mkaply]]

I believe it is IE's behavior.

The customer here is using a navigator.foo variable to store the state of their
application, so when the user hits reload, they know not to go back to the
server and reinitialize the whole application.

Comment 15

[Christopher Aillon (sabbatical, not receiving bugmail)]

Okay after talking with jst, I think I have a handle on how to properly fix
this.  Taking since I'm working on the patch.

Comment 16

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Better fix

Okay, this should do the right thing.  On top of the changes for this bug, I
needed to modify a few things in caps to ensure that the security checks I
added will work properly across domains when we don't have any code on the JS
stack, and are in process of switching pages.  That is included in this patch.

Comment 17

[Mitchell Stoltz (not reading bugmail)]

Comment on attachment 125859 [details] [diff] [review]
Better fix

The patch looks good.

Chris or Johnny, is it OK to release mDocumentPrincipal in SetNewDocument? Are
we using the cached principal for anything else later?

Let me know, but r=mstoltz anyway.

Comment 18

[Christopher Aillon (sabbatical, not receiving bugmail)]

Mitch, we don't use it later on.  Also, note that we already were releasing it
in the same case earlier on in the function.  I just held on to it a bit longer
before releasing it for good so I could run my same origin checks on it.

Comment 19

[Johnny Stenback (:jst)]

Comment on attachment 125859 [details] [diff] [review]
Better fix

+    if (mNavigator) {
+      // Loading a new document.  Compare it's principal against the
+      // old principal to see whether we are allowed to keep the old
+      // navigator object.

Only need to do that if mNavigator && mDocumentPrincipal, shouldn't ever have a
navigator and no old principal, but you never know in this codebase...

+	   if (mNavigator && aDocument) {
+	     // Make our JS wrapper hold on to the navigator object so
+	     // that it doesn't go away when transitioning from page to
+	     // page. We do this by just asking for the navigator
+	     // property on the global object. That causes the global
+	     // object's resolve hook to cache the property on the
+	     // global, and thus root it which prevents GC from getting
+	     // rid of the object.
+
+	     jsval dummy;
+	     ::JS_GetProperty(cx, mJSObject, "navigator", &dummy);
+	   }

This code is now inside an if (mDocument) check, so aDocument will never be
non-null. This code needs to go further down inside the if (mContext &&
aDocument) check.

- In nsGlobalWindow.h:

+  // XXX We need mNavigatorHolder because we make two SetNewDocument()
+  // calls when transitioning from page to page. This keeps a reference
+  // to the JSObject holder for the navigator object in between
+  // SetNewDocument() calls so that its root doesn't get garbage 
+  // collected in between these calls.

Make that "... so that the JSObject doesn't get garbage collected..."

sr=jst with those changes.

Comment 20

[Christopher Aillon (sabbatical, not receiving bugmail)]

Fix checked in with the same origin check discussed.  Mkaply, could you verify
this now works?

Comment 21

[Christopher Aillon (sabbatical, not receiving bugmail)]

Just to clarify what this patch does:

It will keep the user-defined properties of window.navigator if moving in
between pages in the same origin.  Once the window moves to a different origin
however, the properties of navigator are "forgotten".

Comment 22

[Christian :Biesinger (don't email me, ping me on IRC)]

This patch caused bug 209946 (copying text fails)

Comment 23

[Axel Hecht]

caillon, please check all consumers of
nsScriptSecurityManager::CheckSameOriginPrincipal before marking this solved.
This regressed all XSLT security checks for chrome URLs, too, filed bug 210601.
Makes me wonder if p3p is broken these days.

Comment 24

[Christopher Aillon (sabbatical, not receiving bugmail)]

Dude this is fixed.  If you file a new bug, there is no need to reopen this one.
 This bug should only get reopened if this patch gets backed out, which it won't.

Fixed again.

Comment 25

[Christopher Aillon (sabbatical, not receiving bugmail)]

*** Bug 50753 has been marked as a duplicate of this bug. ***