72197 - setting location= in javascript: link replaces current page in session history

Status: VERIFIED FIXED

(Core :: DOM: Navigation, defect, P3)

Description

[Christopher Blizzard (:blizzard)]

Here's a bit of JS that I use to load bugs.  It's in my bookmarks as the
property of the bookmark.  When the resulting url is loaded the back button
isn't highlighted and it hasn't been added to the history as near as I can tell.
 You have to do this with a newly opened browser window.

<DT><A HREF="javascript:id=document.getSelection();if(!id){void(id=promp
t('Show Mozilla bug no.',''))}if(id)location.href='http://bugzilla.mozilla.org/s
how_bug.cgi?id='+escape(id)+' '" ADD_DATE="943845841">Show Bug...</A>

Comment 1

[Claudius Gayle]

session history --> radha. and this is either a dupe or a recent regression.

Comment 2

[Radha on family leave (not reading bugmail)]

Does this JS code in a frameset page or create a frameset page? There's already 
a bug that says, if the first page loaded on a browser window is a frameset 
page, back button fails to work. This bug is assigned to mscott.

Comment 3

[Mike Shaver (:shaver emeritus)]

No, this happens with any javascript: URL, where ``result'' means ``URL load
from the setting of document.location during the execution of a javascript: URL
that doesn't return a non-void result''.

The bookmarklet on the front page of bugzilla is a great example, and this bug
is insanely annoying, for the record.

Comment 4

[Radha on family leave (not reading bugmail)]

This bug is caused by the checkin for bug 39938 in js/dom/src/nsLocation.cpp. 
When the location.href is processed, docshell is still busy with the alert 
dialog, which makes the new url to replace the existing one, instead of a normal 
load. 

Comment 5

[Jesse Ruderman]

If a javascript: bookmark (bookmarklet) or javascript: link loads a URL with 
[document.]location[.href], the new URL replaces the current page in session 
history.  This happens even if the link/bookmarklet doesn't call alert().

Additional URLs with bookmarklets:
http://www.google.com/options/netscape6.html (previously linked to on Google's 
front page)
http://www.squarefree.com/bookmarklets/pagedata.html#validate

Comment 6

[Jesse Ruderman]

Attachment: testcase

Comment 7

[Radha on family leave (not reading bugmail)]

Attachment: Initial patch

Comment 8

[Radha on family leave (not reading bugmail)]

Attachment: PR_FALSE initial value for mInScript in the constructor

Comment 9

[Radha on family leave (not reading bugmail)]

The attached patch takes care of bookmarklets, onLoadHandlers and simple script 
tags that have a location.href in them.

Comment 10

[Alec Flett]

sr=alecf

Comment 11

[Christopher Blizzard (:blizzard)]

Radha, who needs to review this?

Comment 12

[Radha on family leave (not reading bugmail)]

I would like rpotts to take a look at this. I will talk to him today.

Comment 13

[rpotts (gone)]

Hi Radha,

I talked with jst about this one and he suggested that you add a 
IsExecutingEventHandler(PRBool *aResult) to nsIScriptContext.

It turns out that event handlers are executed by calling CallEventHandler(...) 
while other scripts are executed by calling EvaluateString(...)

So, inside on nsLocation you can check 
nsIScriptContext::IsExecutingEventHandler(...) to determine the replacement 
policy.

This seems to limit the number of dependencies and keeps knowledge of script 
evaluation out of the DocShell (and nsDocument) :-)

How does this sound?
-- rick

Comment 14

[Radha on family leave (not reading bugmail)]

That does sound better. As we discussed in the morning, I was trying to use 
nsDocument for maintaining the script execution status and it was more messier 
than the patch I have attached here. I will try jst's suggestion.

Comment 15

[Alec Flett]

one nit - please use GetIsExecutingScript() rather than IsExecutingScript() so
that we can use IDL attributes to describe this. (readonly attribute
isExecutingScript;)

Comment 16

[rpotts (gone)]

Since nsIScriptContext will never be a [scriptable] interface is there any 
advantage to using a 'readonly attribute isExecutingScript' rather than 'boolean 
IsExecutingScript' method?

It seems like GetIsExecutingScript() just makes it harder to understand the 
code...

Comment 17

[Alec Flett]

oh, I didn't realize it would never be scriptable...

Comment 18

[Radha on family leave (not reading bugmail)]

I tried out the suggestion that jst made. But it doesn't work in all situations.
The original test case posted to this bug by blizzard on 3/16 for the
bookmarklet fails. In this case,  the location.href is executed by
EvaluateString(), even though clicking OK in the alert dialog triggers the
location.href. Also, I have to make a special case checking for onLoad handlers,
since we want to do LoadReplace if a onLoadHandler had a location.href in it. 

Comment 19

[Radha on family leave (not reading bugmail)]

Attachment: patch - third attempt

Comment 20

[Radha on family leave (not reading bugmail)]

The latest patch is a combination of first and the sugegstion jst made. Th idea 
is to isolate the <script> tag from the rest and set a flag in nsIScriptContext 
whenever we are in the middle of processing a script tag. This flag is used 
bynsLocation to distinguish a regular load from a replace load. So, all 
location.href will lead to a normal load except for the ones inside a script 
tag. We do not use nsDocument or nsDocShell to pass flags around. 

Comment 21

[rpotts (gone)]

Hi Radha,
This looks good to me (r=rpotts)

The only comment I have is to change my mind about what I said earlier :-)  In
this instance, I think that it would be better to have an attribute called
'processingScriptTag' rather than the IsProcessingScriptTag and
SetProcessingScriptTag methods...  So, for consistancy the methods should be
called GetProcessingScriptTag and SetProcessingScriptTag

I admit that I was wrong about the names - and now it's come back to bite me :-)

Comment 22

[Christopher Blizzard (:blizzard)]

sr=blizzard

Comment 23

[Johnny Stenback (:jst)]

Whoa, what's up with the new argument to EvaluateScript() in the HTML content
sink? Is the idea that external vs. inline scripts should be treated differently
here, that seams really really wrong to me. If EvaluateScript() is called in the
sink, you know if's because a <script> tag was seen in the HTML that's loaded,
is that not all you're interested in? Also, the attached patch only fixes the
problem for HTML, not for XHTML, you need similar changes in the XML content
sink, having the two work differently doesn't seem like a good idea to me.

Also, to add to the fun, Vidur is rewriting the script loading code and taking
it out of the content sinks all together, so depending one who's checking in
first, changes will be needed in both these diffs, and in Vidur's diffs.

Cc:ing Vidur, please don't check this in yet until we really know what we wanto
do here.

Comment 24

[Johnny Stenback (:jst)]

Hmm, I see this was checked in already, please don't close this before dealing
with the differences between inline vs. external scripts, which I guess is
something Vidur will haveto fix now. Oh well... 

Comment 25

[Radha on family leave (not reading bugmail)]

jst, the addition argument to EvaluateScript() got added because, in the 
original patch, it was getting called with a TRUE flag by ProcessScriptTag() and 
with a FALSE flag by OnStreamComplete(). Just minutes before I checked in, I saw 
your reply to the email from rpotts that it needs to be TRUE for both the cases. 
So, now, like you said, since both the callers pass a TRUE flag, the additional 
argument to EvaluateScript() is not needed. I can take that out and make similar 
changes to nsXMLContentSink.

Vidur, What is the status of your rewrite? Will you be checking in RSN? Thanks,

Comment 26

[Radha on family leave (not reading bugmail)]

vidur's changes has the patch attached to this bug. Markign fixed.

Comment 27

[sairuh (rarely reading bugmail)]

mass-verifying claudius' Fixed bugs which haven't changed since 2001.12.31.

if you think this particular bug is not fixed, please make sure of the following
before reopening:

a. retest with a *recent* trunk build.
b. query bugzilla to see if there's an existing, open bug (new, reopened,
assigned) that covers your issue.
c. if this does need to be reopened, make sure there are specific steps to
reproduce (unless already provided and up-to-date).

thanks!

[set your search string in mail to "AmbassadorKoshNaranek" to filter out these
messages.]

Comment 28

[Boris Zbarsky [:bzbarsky]]

This checkin caused bug 178729 -- broken history when the script is running in
one window and setting location in a _different_ window.