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

VERIFIED FIXED in mozilla1.3final

Status

()

Core
Document Navigation
P2
major
VERIFIED FIXED
15 years ago
10 years ago

People

(Reporter: Basil Fritts, Assigned: Darin Fisher)

Tracking

({regression})

Trunk
mozilla1.3final
regression
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed1.3, URL)

Attachments

(1 attachment)

(Reporter)

Description

15 years ago
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.
Radha, this is all yours.
Assignee: rogerl → radha
Component: JavaScript Engine → History: Session
QA Contact: pschwartau → kasumi

Comment 2

15 years ago
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

15 years ago
this looks like a duplicate of bug 190637
(Reporter)

Comment 4

15 years ago
*** Bug 190637 has been marked as a duplicate of this bug. ***
(Reporter)

Comment 5

15 years ago
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."
Summary: Back/Forward buttons break when a variable is used in changing the value of location.href → Back/Forward buttons break when a variable is used in a javascript: URI that changes the value of location.href

Updated

15 years ago
Flags: blocking1.3?

Comment 6

15 years ago
See also: bug 192793 -- the same problem?
(Reporter)

Comment 7

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

Comment 8

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

Updated

15 years ago
Flags: blocking1.3? → blocking1.3-

Comment 9

15 years ago
re-nominating.
Flags: blocking1.3- → blocking1.3?
darin, any chance this is related to your async streams stuff that landed around
the time this started showing up?
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.
Assignee: radha → darin

Comment 12

15 years ago
This breaks a lot of bookmarklets. We should try to get this for 1.3
Flags: blocking1.3? → blocking1.3+

Comment 13

15 years ago
Brendan says the bookmarklet is buggy. He'll explain because I'd just confuse
things. Not a 1.4 blocker. 
Flags: blocking1.3+ → blocking1.3-
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

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

Comment 16

15 years ago
brendan: thanks for all the info!
Severity: normal → major
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → mozilla1.4alpha
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
(Reporter)

Comment 18

15 years ago
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

15 years ago
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.
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 drivers@mozilla.org
would not hold 1.3 up for a fix to this bug.

/be

Comment 21

15 years ago
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

15 years ago
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

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

http://www.google.com/options/netscape6.html
Cc'ing bclary -- see comment 23.

/be

Comment 25

15 years ago
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.

(Assignee)

Comment 26

15 years ago
Created attachment 115964 [details] [diff] [review]
v1 patch

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

Comment 27

15 years ago
Comment on attachment 115964 [details] [diff] [review]
v1 patch

trivial patch... i think we should try to get this into 1.3 final.
Attachment #115964 - Flags: superreview?(brendan)
Attachment #115964 - Flags: review?(dougt)
(Assignee)

Comment 28

15 years ago
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.
(Assignee)

Comment 29

15 years ago
re-nominating for 1.3 final.  this is a really trivial patch.
Flags: blocking1.3- → blocking1.3?
Target Milestone: mozilla1.4alpha → mozilla1.3final
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
Attachment #115964 - Flags: superreview?(brendan) → superreview+
(Assignee)

Comment 31

15 years ago
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.
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
(Assignee)

Comment 33

15 years ago
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

15 years ago
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.
Attachment #115964 - Flags: review?(dougt) → review+
(Assignee)

Comment 35

15 years ago
Comment on attachment 115964 [details] [diff] [review]
v1 patch

seeking drivers approval to land this for 1.3 final.
Attachment #115964 - Flags: approval1.3?
Comment on attachment 115964 [details] [diff] [review]
v1 patch

a=blizzard
Attachment #115964 - Flags: approval1.3? → approval1.3+
(Assignee)

Comment 37

15 years ago
fixed1.3
fixed-on-trunk
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Flags: blocking1.3?
Resolution: --- → FIXED

Updated

15 years ago
Whiteboard: fixed1.3

Comment 38

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

Comment 39

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

Comment 40

15 years ago
WFM in a current build, no new reports for a while; verifying fixed.
Status: RESOLVED → VERIFIED

Updated

15 years ago
QA Contact: kasumi → marina

Updated

10 years ago
Component: History: Session → Document Navigation
QA Contact: marina → docshell
You need to log in before you can comment on or make changes to this bug.