BODY onUnload JavaScript gets wrong value for location.href

VERIFIED FIXED in mozilla0.9.3



18 years ago
17 years ago


(Reporter: sdh4, Assigned: rpotts)


({privacy, testcase})

privacy, testcase

Firefox Tracking Flags

(Not tracked)


(Whiteboard: PDT+, URL)


(2 attachments)



18 years ago
From Bugzilla Helper:
User-Agent: Mozilla/5.0 (X11; U; Linux 2.2.16-3 i686; en-US; rv:0.9.1)
BuildID:    2001060713

When the onUnload JavaScript is executed, location.href is the address of the
new web page, not the old web page. This allows the old web page to spy on where
you go to after you leave it (even if you leave by typing in a URL). 

This bug should be severity: security, except that classification does not exist
(bug #84746)

Reproducible: Always
Steps to Reproduce:
1.go to (one of many sites run by
2. Wait for an auto-redirect
3. Save the HTML for this webpage in a temporary place
4. go to your home page
5. A tiny hidden window has popped up (it may be difficult/impossible
to find. I can find it easily (~75x75) on Linux with IceWM window manager and
the GNOME pager. It usually hides on the left edge or lower right corner of the
6. Right click on the hidden window and view page info. You will see your
homepage location encoded into its URL.
7. Look at the source you saved in step 3. In particular the BODY onUnload and
the function MakeRemoteWindow().
8. Quit your browser to get rid of the obnoxious JavaScript web bug. 

Actual Results:  I got my homepage location URL encoded into the web bug window
that popped up.

Expected Results:  Encoded the referring page (

<BODY leftMargin="20" topMargin="0" marginwidth="20" marginheight="0"

function MakeRemoteWindow() {

 // allow disabling of console
 if (remote == 1){

 var baseurl = '';
 var console = baseurl +
 '?BPROGRAM=' + program + 
 '&' + 
 'LINKIN=' + linkin +
 '&' +
 'REF=' + location.href;

 H =,"ef","height=1,width=1,left=10000,top=10000,location=0,

Yikes!  over to DOM 0, ccing mstoltz and hyatt (since I recall hyatt doing
things with keeping the old doc in place till the new one comes in).

Steve, thanks for the excellent bug report.  Our apologies for taking so long to
get to it.
Assignee: joki → jst
Component: DOM Events → DOM Level 0
Ever confirmed: true
Keywords: privacy
QA Contact: vladimire → desale

Comment 2

18 years ago
Created attachment 41083 [details]


18 years ago
Keywords: testcase
OS: Linux → All
Hardware: PC → All

Comment 3

18 years ago
Think bug 88684 is related to this.
This should be fixed for mozilla0.9.3.
Keywords: mozilla0.9.3
Target Milestone: --- → mozilla0.9.3
Ok, so I talked to Rick about this and he did some investigation and thinks we
can simply move the fireing of the onunload handler earlier in the code so that
it's fired while window.location still has the value of the old page. Reassigning.
Assignee: jst → rpotts

Comment 6

18 years ago
Created attachment 42324 [details] [diff] [review]
Patch to fire UnLoad() event before changing any state in the DocShell...

Comment 7

18 years ago
I forgot to mention that the patch is for the trunk :-)
nsDocShell::FireUnloadNotification() never uses 'rv', maybe remove it?

Do we really care about 'rv' at line ~3815?

In stead of making mFiredUnloadEvent a PRBool cound't you make it a PRPackedBool
and squeese it in with other smaller than 32-bit members?

Looks like we could loose 'rv' in DocumentViewerImpl::Unload() too.

Nit of the day, in nsDocShell::FireUnloadNotification():

+    if (mContentViewer && !mFiredUnloadEvent) {
+      mFiredUnloadEvent = PR_TRUE;
+        rv = mContentViewer->Unload();

Fix the indentation just below the if...
Other than that, sr=jst

Comment 9

18 years ago
I guess that in nsDocShell::FireUnloadNotifications(...) i'm not really sure
what the 'failure' policy should be :-)

One option is to fail if any of the nsIContentViewer::Unload() calls fail...
Another option is to always ignore the result code and continue notifying...

I have no idea which is the best option :-)  So, for now, I was capturing the
return code but ignoring it - best of both worlds :-)

I've made changes to ignore the return code from FireUnloadNotifications...

I was afraid to lump in the PRBool -> PRPackedBool changes into this patch
because it needs to land on the branch too :-)

I'll open up another bug to address the PRPackedBool issue specifically.

-- rick
This is Nisheeth typing at jst's computer.  r=nisheeth.

Comment 11

18 years ago
I've checked the changes into the trunk...  I'm just waiting for approval to
check them into the branch...
Adding nsBranch keyword to get more eyeballs on this security bug.
Keywords: nsBranch

Comment 13

18 years ago
The best (only, probably) way to do it is to e-mamil

Comment 14

18 years ago
There's a whole lot going on in that patch.  Is there a simple version that
could be made for the branch?  Something like "deleted call from old place and
inserted it in new place"?  I'm hesitant to plus the existing patch.

Comment 15

18 years ago
hey steve,

believe it or not, that's basically what this patch is :-)

In the current implementation, nsWebShell.cpp has the code to iterate across
DocShells and fire the OnUnload(...) event- FireUnloadForChildrten(...).  This
code as been moved into nsDocShell.cpp as the FireUnloadNotification(...) method.

I needed to move this code into the DocShell, because this method needs to be
called from the middle of a method within nsDocShell.cpp

The other big part of the patch is moving the code that used to be in
nsWebShell::FireUnloadEvent(...) into nsDocumentViewer::Unload(...).  I decided
to do this (Rather than move it into the nsDocShell) because the OnLoad() event
is fired from the nsDocumentViewer too... and it is easier to get to the
information required to fire the event at the nsDocumentViewer level (than at
the nsDocShell/nsWebShell level).

Finally, the call to FireUnloadNotification() got moved from
nsWebShell::SetupNewViewer(...) into he middle of
nsDocShell::CreateContentViewer(...).  This is the critical part of the patch
(ie. the part that actually fixes the bug).  The rest of the patch is just the
housekeeping required to make the fix...

Comment 16

18 years ago
Oops, I forgot to add myself to the CC list and am only just seeing your
response :-(

Adding PDT+, please check in on the branch.  What can we do to ensure complete
confidence in this fix?  Do we have a targetted set of tests?  Can we invent a
set of tests?  Have people been stressing this area with trunk builds since the
patch went in?
Whiteboard: PDT+

Comment 17

18 years ago
The upshot of the fix is the thew OnUnload(...) script handler will be fired
earlier than before - assuming that all of the code is correct :-)

jst and i lxr'ed through the codebase to see what code relies on the
OnUnload(...) event... it looked like some of the wizards were the only consumers...

So, i think that if the Profile wizard and the mail account wizard do not show
regressions, then we should be in good shape...

I haven't heard of any regressions in the wizards since i landed this patch on
the trunk...

Comment 18

18 years ago
I've just checked the patch into the branch.
Last Resolved: 18 years ago
Resolution: --- → FIXED

Comment 19

18 years ago
Verified with 2001-07-24-06 branch.

Comment 20

18 years ago
Is there a connection to bug 88684 ?

Comment 21

17 years ago
See also bug 145579, another privacy bug with the same impact.
You need to log in before you can comment on or make changes to this bug.