Closed Bug 163645 Opened 22 years ago Closed 21 years ago

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


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

Not set





(Reporter: aniapte, Assigned: caillon)



(Keywords: dataloss, regression)


(3 files, 1 obsolete file)

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 = 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:
Attachment #96013 - Attachment description: First load this. → Testcase file 1. Download both the testcase files to the same directory and then load file 1.
Attachment #96014 - Attachment description: Open this attachment after the first one has been loaded. → Testcase file 2 - Open this attachment after the first one has been loaded.
HOW TO RECREATE: Load the 1st attachment
This creates a navigator object property, and sets it to 1122334455

Now, in the same window load the second attachment
Clicking on the button shows an alert with the value of
If bug exists, value of will be undefined, else, it should say
Confirming. This appears to be a regression from bug 150087. Commenting out the
NS_RELEASE(mNavigator) in nsGlobalWindow.cpp makes the testcase work.
Ever confirmed: true
Keywords: regression
Mass-reassigning bugs to
Assignee: jst → dom_bugs
Seems like dataloss to me, adding keywords and upping severity. We should get to
this ASAP.
Severity: normal → critical
Keywords: dataloss, nsbeta1
Attached patch Probable fix (obsolete) — Splinter Review
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...
adt: nsbeta1-
Keywords: nsbeta1nsbeta1-
This is a big one for one of our customers. Very small change...
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... 
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...
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.
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?
I believe it is IE's behavior.

The customer here is using a 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.
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.
Assignee: dom_bugs → caillon
Attached patch Better fixSplinter Review
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.
Attachment #124476 - Attachment is obsolete: true
Attachment #125859 - Flags: superreview?(jst)
Attachment #125859 - Flags: review?(mstoltz)
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.
Attachment #125859 - Flags: review?(mstoltz) → review+
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 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.
Attachment #125859 - Flags: superreview?(jst) → superreview+
Fix checked in with the same origin check discussed.  Mkaply, could you verify
this now works?
Closed: 21 years ago
Resolution: --- → FIXED
Summary: User defined properties of Javascript navigator object are not remembered wneh a new page is loaded. → User defined properties of Javascript navigator object are not remembered when a new page is loaded.
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".
This patch caused bug 209946 (copying text fails)
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.
Resolution: FIXED → ---
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.
Closed: 21 years ago21 years ago
Resolution: --- → FIXED
*** Bug 50753 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.