193887 - Back/Forward buttons break when a variable is used in a javascript: URI that changes the value of location.href

Status: VERIFIED FIXED

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

Description

[Basil Fritts]

When changing the value of location.href and a variable is in the string, as in
the URL above, the URI pointed to by location.href is not accessible by the back
or forward buttons.

Steps to reproduce:
1. execute the javascript: pseudo-URI above.
2. follow any link on the page.
3. try to go back to the google page.

Expected: to be able to go back
Actual: nothing happens

Also note that the generated URI is printed in plain in the browser viewscreen.
This seems clearly related, but if it is not, it is a polish issue and should
have its own bug.

The JS Console prints this error:
Error: [Exception... "Component returned failure code: 0x80004005
(NS_ERROR_FAILURE) [nsIURI.hostPort]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"
 location: "JS frame :: chrome://navigator/content/nsBrowserStatusHandler.js ::
anonymous :: line 350"  data: no]
Source File: chrome://navigator/content/nsBrowserStatusHandler.js
Line: 350

When trying to use the back button (or forward, if you are prior in the
history), the JS console reports this:
Error: uncaught exception: Permission denied to set property Window.foo

I have seen this in the following builds:
2003021007-trunk, MachO for Mac OS X
2003021007-trunk, for Linux
2003021007-trunk, Phoenix for Linux
Mozilla 1.3b for Mac OS X and Linux

This affects several bookmarklets that I use on a daily basis.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Radha, this is all yours.

Comment 2

[Tim Powell]

This is quite reproducable with Mozilla 1.3b on windows as well. Thanks for 
tracking this down, Basil. I hadn't made the connection that it is bookmarklet 
related. It just seemed like every once in a while I couldn't go back/forward 
to pages.

Comment 3

[Ron Thigpen]

this looks like a duplicate of bug 190637

Comment 4

[Basil Fritts]

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

Comment 5

[Basil Fritts]

From the original report on bug 190637:
"I think this broke right around the time that 'location.href="URL"' started
displaying the URL in the content window, before navigation."

Comment 6

[Dan Price]

See also: bug 192793 -- the same problem?

Comment 7

[Basil Fritts]

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

Comment 8

[R.K.Aa.]

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

Comment 9

[Asa Dotzler [:asa]]

re-nominating.

Comment 10

[Brian Ryner (not reading)]

darin, any chance this is related to your async streams stuff that landed around
the time this started showing up?

Comment 11

[Brian Ryner (not reading)]

This broke between 15:00 and 21:00 on 1/17/2003 (testing using commercial
nightlies), which leaves darin's async streams checkin as a very likely candidate.

-> darin.

Comment 12

[Asa Dotzler [:asa]]

This breaks a lot of bookmarklets. We should try to get this for 1.3

Comment 13

[Asa Dotzler [:asa]]

Brendan says the bookmarklet is buggy. He'll explain because I'd just confuse
things. Not a 1.4 blocker. 

Comment 14

[Brendan Eich [:brendan]]

Darin, I have no idea how your changes could have busted things, but here's what
we know (bryner demo'd this to dougt and me, then I had him fix his bookmarklet
and that worked around the history-related bug):

1. let bm be a bookmarklet of the form javascript:location.href='http://wahoo.com'
2. type bm into the location toolbar
3. see a document of type text/plain with content "http://wahoo.com" load for a
split second
4. see http://wahoo.com load
5. click a link on that page, say to http://hahoo.com
6. see http://hahoo.com load
7. click Back, nothing happens
8. try to use the Go menu to go back -- you have to skip back to before step 2's
session history entry

If you fix the bookmarklet to be of the form
javascript:void(location.ref='http://yahoo.com') or similar (tacking a void 0;
at the end works too, but is less elegant), then all is better: step 3 above is
avoided, and the bad state at step 7 is also avoided.

I *think* (bryner or someone willing to do the above can verify) that after step
3 there was no session history entry for the text/plain document synthesized
from the non-void result "http://yahoo.com" of the buggy bookmarklet's
javascript: URL.  That may provide an important clue.

Background, SIAK: A javascript: URL has the form javascript:<program> where
<program> is a JS program.  If that program's last expression-statement result
has void type (is undefined), then the JS URL does not cause a synthesized page
to be loaded -- instead, the JS URL operates on the current page's DOM and does
not unload that page or change session history.

If OTOH the JS URL's program ends its execution with an expression-statement of
non-void type, say a string URL result of assigning 'http://yahoo.com' to
location.href, then that result value is converted to string type, and the
string is examined for a leading '<'.  If it begins with '<', it is loaded as
the content of type text/html, creating a new document that unloads the previous
page and advances session history.  If it doesn't begin with '<', the same thing
happens but the type is text/plain.

So the buggy bookmarklets fail to use the void unary operator to suppress a
useless intermediate page from being loaded.  That causes a bug that we should
fix for 1.4a to bite, namely the bug reported here.

/be

Comment 15

[logan]

Ah void(0);, many thanks! My google search is happy now. :D

Comment 16

[Darin Fisher]

brendan: thanks for all the info!

Comment 17

[Brendan Eich [:brendan]]

More on non-void javascript: URLs, and on pages created completely or partially
by document.write:

In Nav 3 and 4, the content generated by these mechanisms was piped through a
cache converter that stored the tag stream in a cache file identified by a
wysiwyg: URL, so named because without this cache file, in Nav 2, a generated
page was not restored to what you saw, when you returned to it via the Back
button -- if the javascript: URL generated a new fortune cookie message each
time it was evaluated, you would miss that witty fortune that you wanted to see
via Back, and get a dull, new one instead.  That bug was fixed by wysiwyg: URLs
(although they had a few bugs of their own in Nav 3 ;-).

The original, generatING URL was encoded via suffixing in the wysiwyg: URL,
whose prefix host-part contained a document id and a path through a
frameset/frame hierarchy.  The wysiwyg: URL uniquely identified the generatED
document whose source was saved in the cache.

Mozilla has wyciwyg: URLs, based on wysiwyg: URLs.  Are they used to save the
document generated by a javascript: URL?  They ought to be.

/be

Comment 18

[Basil Fritts]

Should this bug be added to the release notes for 1.3? Brendan's comments in
Comment #14 explain the problem well and would be perfect for use in the release
notes after polishing. In addition to some trivial spit-shining, to Brendan's
comments I would add that the use of a variable is necessary to force this bug
to present. Simply changing location.href is not sufficient.

Comment 19

[Tim Powell]

Brendan said:
[T]he buggy bookmarklets fail to use the void unary operator to suppress a
useless intermediate page from being loaded.  That causes a bug that we should
fix for 1.4a to bite, namely the bug reported here.

I find it hard to call the bookmarklet buggy when this bug shows Mozilla made a 
major change from what has been standard behavior for a long time. 
Location.href=url should not create an intermediate page, but should instead 
immediately switch to the new URL. Not only does this break the back button for 
many bookmarklets, but also most every web page using a javascript URL with 
location.href, which is not uncommon in web applications. I'm glad Brendan 
agrees it should be fixed (and thanks for the detailed information; it helps 
work around the problem in bookmarklets, but that's harder to do for web-
applications). Because of the significant change and 3 duplicates in 10 days I 
feel like this should be a 1.3 blocker as Asa originally suggested.

Comment 20

[Brendan Eich [:brendan]]

Tim, can you verify that in 4.x (any platform, esp. Windows; more are better)
there is no intermediate page, of content-type text/plain, containing the URL
assigned to location.href?  I mean, not only that no such page is visible for a
split second, but that no session history entry is created for it?

If so, then I agree there is a bug here that I didn't address in my previous
comments: assigning to location.href from a non-void javascript: URL should
cancel the load of the synthetic document that would have been generated from
converting the non-void result value to string.  The trouble is, I've forgotten
too much of what I wrote in the old 2-4.x codebase to remember what should happen!

Let me look at http://lxr.mozilla.org/classic/ and report back, but please do
confirm 4.x behavior here.

Whether 1.3 should hold for this bug can be debated, of course, but if all the
bookmarklets were fixed, we wouldn't hold it -- and since there is a workaround
in the form of a bona-fide bookmarklet fix, I as one of [e-mail]
would not hold 1.3 up for a fix to this bug.

/be

Comment 21

[Kim Scarborough]

I've been using bookmarlets in this format ever since the earliest days of 4.0,
and I never had this problem, FWIW.

I didn't mind changing them all to the "void" format, but there definitely
should be a big note in the release notes about this issue when 1.3 comes out.

Comment 22

[Doug Turner (:dougt)]

i am not sure if this helps at all, but "foo" is being evaluated as Window.foo
on the second load.  Permission is denied and the load fails.

Comment 23

[Dan Price]

Perhaps there needs to be an advocacy bug to at least get the 'google buttons'
corrected?

http://www.google.com/options/netscape6.html

Comment 24

[Brendan Eich [:brendan]]

Cc'ing bclary -- see comment 23.

/be

Comment 25

[Doug Turner (:dougt)]

just to clarify what I was seeing in the debugger (i didn't spend that much time
looking at it)

you click on the bookmark.  We eval the js program and the result returned is
http://www.mozilla.org.  During this evaluation, the current load is canceled
(the current load is the jschannel), and we proceed to load mozilla.org.  

When we press the back button (in the failing case - after we go to another page
and press back), session history tries to reload the jsurl.  This second
evaluation results in an exception "Permission denied to set property
Window.foo".  no clue why.

Comment 26

[Darin Fisher]

Attachment: v1 patch

trivial patch.	nsInputStreamChannel::Cancel was mistakenly not setting mStatus
on cancel if !mPump.

Comment 27

[Darin Fisher]

Comment on attachment 115964 [details] [diff] [review]
v1 patch

trivial patch... i think we should try to get this into 1.3 final.

Comment 28

[Darin Fisher]

to clarify: my patch restores our old behavior, in the case that a cancel was
called when the channel is not pending.  technically speaking, cancel is not
permitted while the channel is not pending.  however, it certainly seems like
something is depending on this.  i haven't identified the something.

Comment 29

[Darin Fisher]

re-nominating for 1.3 final.  this is a really trivial patch.

Comment 30

[Brendan Eich [:brendan]]

Comment on attachment 115964 [details] [diff] [review]
v1 patch

Any similar losses of cancel-status stickiness in the async landing?  Just
asking -- thanks for fixing.

We should keep this bug open till the mystery is (or all mysteries are) solved,
for sure.

/be

Comment 31

[Darin Fisher]

ok, so the problem is this bit of code in nsJSChannel::AsyncOpen,

    rv = mIOThunk->EvaluateScript(mStreamChannel);
    if (NS_SUCCEEDED(rv)) {
        rv = mStreamChannel->AsyncOpen(aListener, aContext);
    } else {
        // Propagate the failure down to the underlying channel...
        (void) mStreamChannel->Cancel(rv);
    }

this is where it calls Cancel on a channel that is not pending.  this code
really needs to be reworking.  i think the Cancel is critical because |this|
will have already been added to the loadgroup.  by Cancel'ing mStreamChannel,
the status of |this| will be the status of mStreamChannel (just look at
nsJSChannel::GetStatus).  as a result, the loadgroup observer will see a
canceled request in the loadgroup and not bother creating a new docshell, etc.

looks like nsFileChannel is the only other channel implemented similarly.  i
don't think we need to worry about fixing nsFileChannel though since the problem
here is that nsJSChannel is depending on an undocumented (invalid even) behavior
of nsInputStreamChannel.

Comment 32

[Brendan Eich [:brendan]]

nsJSProtocolHandler.cpp is old and probably needs some arin-love.  Feel free to
whack it heavily ASAP.

Is 1.3 good with just the v1 patch?  I'd love to see nsJSPH updated and reworked
for 1.4a as needed, if you and dougt can do it or help me do it.

I should look into how wyciwyg: URLs work in Mozilla.  Anyone who knows more,
please educate me and others in this bug.

/be

Comment 33

[Darin Fisher]

brendan: yes, this v1 patch is good enough for 1.3 final.  the next step can
definitely wait for 1.4 (see bug 195562).

Comment 34

[Doug Turner (:dougt)]

Comment on attachment 115964 [details] [diff] [review]
v1 patch

brendan is right - we need ot clean up the nsJSChannel implementation.	

This patch fixes the underlying problem, but calling cancel on a channel that
hasn't been opened is bad form.

Comment 35

[Darin Fisher]

Comment on attachment 115964 [details] [diff] [review]
v1 patch

seeking drivers approval to land this for 1.3 final.

Comment 36

[Christopher Blizzard (:blizzard)]

Comment on attachment 115964 [details] [diff] [review]
v1 patch

a=blizzard

Comment 37

[Darin Fisher]

fixed1.3
fixed-on-trunk

Comment 38

[R.K.Aa.]

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

Comment 39

[R.K.Aa.]

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

Comment 40

[Michael Lefevre]

WFM in a current build, no new reports for a while; verifying fixed.