25821 - javascript: URL links with TARGET not processed correctly.

Status: RESOLVED WORKSFORME

Description

[Richard d'Avrincourt]

I am using Mozilla to access an Oracle Application and it
gives out those bugs:

1. There are additional text that are displayed each time tables are
used. The texte are '1x1space'. The text could have been there
for testing purposes only... Just in case. I prefer mentioning that
2. The bottom frames of the application use different colors. The
'green' colour is displayed properly while there is a bug displaying
'blue'

Hope this helps

Comment 1

[Richard Zach]

The problem appears on the third page in this site: click all check marks, click
continue, then continue again.  The "1x1space" appear where spacer gifs named
"1x1space.gif" should be displayed. 

Comment 2

[Dan Rosen]

I've had this problem before as well, on sites such as
http://www.browndailyherald.com/.

It seems as though spacer gifs (perhaps spacer images in general? unlikely...)
don't like being rendered. The primary characteristic of spacer gifs is that all
their pixels (no matter what size the image is) are transparent. Perhaps there
is code in Moz which doesn't grok this and falls back by displaying the image name.

Comment 3

[karnaze (gone)]

After marking all of the checkboxes and hitting the continue button, I'm getting 
a javascript error and nothing happens. I don't know if the html is bad or it is 
a javascript error. Reassigning to Norris.

JavaScript Error: ReferenceError: go_forward is not defined

Comment 4

[Norris Boyd]

Change component to DOM and reassign.

Comment 5

[Dan Rosen]

Okay, here's the story:

There are two simplest testcases: one is when the image is missing (in the case 
of browndailyherald.com, or my own sample, 
http://www.thirdnipple.com/mozilla/missing_image.html) or when the image is 
broken (in the case of 
http://www.national.com.au:8001/apphloan/bin/g_images/1x1space.gif, which is a 
way to get that image without surfing around).

In other words, I don't believe it's a DOM bug at all. It's more of an 
"indecision" bug -- they didn't want to display the "broken-image" image, and 
didn't want to display nothing at all (which is what NS4 does), so they 
displayed the first part of the filename instead.

All that needs to be done is to come to a decision about what to display if the 
image is busted or missing. Fixing the bug after that should be trivial.

Comment 6

[vidur (gone)]

The javascript problem that Chris Karnaze mentioned seems to occur because the 
bottom frame of the page has a link with javascript: URL with a TARGET 
attribute. We're currently ignoring the TARGET attribute. I don't know if this 
contributes to the problem reported originally.

I'm going to change the Summary to reflect the javascript: problem.

Comment 7

[Dan Rosen]

Vidur, these are two distinct bugs: the javascript bug currently reflected in 
the summary (reported by karnaze) and the broken/missing image bug 
(reported by [e-mail]) which I've found testcases for.

Somebody ought to separate these bugs, but I don't know whether there is a 
standard way of doing this...

Comment 8

[ekrock's old account (dead)]

Nominating nsbeta2. We have to start drawing a line on DOM0 backward 
compatibility; these bugs are supposed to be a high priority for nsbeta2 per the 
beta2 criteria.

Comment 9

[leger]

need to add a "missing image" bug to split off that problem. ekrock, can you do 
this?

Comment 10

[Kurt Weinschenker]

Tested above testcases tonight.  This bug also shows up on Mac OS build M16-

2000052408.   Perhaps this should be marked as an "All" platform bug?

iMac DV, Mac OS 9.0.4, MRJ 2.2, carbon libs 1.0.4.

Comment 11

[shrirang khanzode]

The 'broken/missing image problem' has been seperately reported. 
Pls use bug 42364 to continue discussion on that problem .Thnx.

Comment 12

[Mike]

M16 has been out for a while now, these bugs target milestones need to be 
updated.

Comment 13

[ekrock's old account (dead)]

If we aren't going to fix this for nsbeta2, then this should *** definitely *** 
be an nsbeta3 stopper. b.c. with DOM0 event handling. No brainer that we have to 
fix this before FCS.

Comment 14

[Nisheeth Ranjan]

Re-assigning to mscott for further analysis as per the decision in our bug 
triage meeting.

Comment 15

[Scott MacGregor]

*ahem* unfortunately at the url specified by this url, layout is no longer
laying out the continue button so I can't reproduce the steps karnaze was
following. 

Comment 16

[ekrock's old account (dead)]

Escalating to P2 as this is high profile backward compatibility. javascript: 
URLs with TARGET for links have been supported since Nav2 IIRC. As such, this is 
eligible for check-in through 9/21. mscott, do you think you could fix by then? 
If so, please accept as [nsbeta3+].

Comment 17

[selmer (gone)]

comparing priority of this w/other mscott bugs...

Comment 18

[ekrock's old account (dead)]

mscott is overloaded. Can anyone lend a hand with this or suggest another person 
who could? If we don't get this fixed, javascript: URL links with a designated 
TARGET won't be processed correctly for RTM. Bad, as this is JS 1.0 DOM0 
backward compatibility. Help!!! Marking helpwanted. cc:ing folks who might be 
able to help or suggest someone who can.

Comment 19

[Brendan Eich [:brendan]]

Anyone, mscott especially: if you make the URL a non-javascript: one, does the 
targeted window or frame load?  If so, what's different about javascript: URL 
handling?  Any pointers to code (LXR links) would be much appreciated.  I have 
not dived into the nsJSProtocolHandler.cpp pool in a while.

/be

Comment 20

[Scott MacGregor]

Here's the problem Brendan: The docshell evaluates JS urls in a special fashion
before we even invole the uriloader. The act of opening a JS channel actually
forces evaluation of the JS. Warren tried to change this behavior in the
JSProtocolHandler but ran into a whole slew of problems (I don't remember them
now) and was forced to go back to the current behavior.

Before the uriloader is invoked, we create a channel for the URI. In the normal
case, we then pass this channel into the uriloader which opens the channel,
discovers the content type and retargets the output to the correct consumer.
However, the JS Protocol Handler evalues the JS when you create the channel. So
the result has already been determined before the uriloader is even involed. In
fact, we don't involve the loader at all if the result of the channel creation
was: NS_ERROR_DOM_RETVAL_UNDEFINED.

Here's the relevant code in docshell: nsDocShell::DoURILoad

   // open a channel for the url
   nsCOMPtr<nsIChannel> channel;
   nsresult rv;
   rv = NS_OpenURI(getter_AddRefs(channel), aURI, nsnull, loadGroup,
                     NS_STATIC_CAST(nsIInterfaceRequestor*, this));
   if(NS_FAILED(rv))
      {
      if(NS_ERROR_DOM_RETVAL_UNDEFINED == rv) // if causing the channel changed the
         return NS_OK;                        // dom and there is nothing else to do
      else
         return NS_ERROR_FAILURE;
      }

Somehow I got taken off the bug list, cc'ing myself again.

Comment 21

[Brendan Eich [:brendan]]

Thanks mscott (sorry about not cc'ing you, I took the bug -- hope you don't mind
that ;-).

Without reopening the can of worms that warren closed by having the JS
evaluation take place at channel creation time, I bet we can fix this: don't we
know the TARGET attribute?  In the case of a non-javascript: URL, how does the
URI loader discover that another window is being targeted?

/be

Comment 22

[Scott MacGregor]

The uriloader has a private method called GetTarget:
http://lxr.mozilla.org/seamonkey/source/uriloader/base/nsURILoader.cpp#509

this method does the work of finding the right window context to open the url
inside of before it calls AsyncRead.

Presumably, this method could be made public and the JS ProtocoolHandler could
attempt to call through it to get the right window context (which can be
GetInterace'd for a docshell) before evaluating the JS.

Comment 23

[lchiang]

B3 has passed.  Clearing "b3 need info"

Comment 24

[selmer (gone)]

Seems like this should be rtm-.  Is there an argument that huge numbers of 
people will experience this bug?  It looks like a great thing to get on the 
trunk though.  Please update the status whiteboard to indicate your desire to 
get into RTM or not.

Comment 25

[tompkins]

We are having this exact same problem with BuyerXpert 4.0, and if not fixed 
would prevent us from supporting Netscape 6 with our Bx4.0 release on 11/30.  We 
use the target field to update information from our catalog order frame on the 
right to the shopping cart frame on the left.  The left frame does not get 
rendered with the updated information.

Would like priority reset to P1 and severity as blocker

Comment 26

[tompkins]

To recreate the problem here's the URL

http://maidstone.red.iplanet.com:9090/NASApp/buyer/Login
enter mercury.com
login as bugsbunny1
password is bugsbunny1
click on buy
click on following links in order general catalog, office furniture, book01
click on first select box
click on add to cart

At this point the left hand frame does not get rendered with the shopping cart 
information that should reflect the item that has been added to the cart.

Comment 27

[Johnny Stenback (:jst)]

Tompkins, I followed your instructions and I do get the item in the left frame
when, am I missing something?

Comment 28

[Johnny Stenback (:jst)]

Brendan, a real fix for this problem will require a redesign of the way we do
link targetting in the docshell/uriloader, the way it's done today seems just
really really wrong. The problem is that when you click on a link today we tell
the docshell (the one that the link is displayed in) that the link was clicked,
then the docshell does it's URI loading (including mucking with session history
for the docshell where the link is, even if the link has a target!!!), initiates
the URL load and passes the URL load, including the target, off to the
uriloader. Once the uriloader gets a responce from the net/file it looks for the
target for the URL load and if there's no target it opens up a new window and
hands the data off to that window.

The problem with javascript: URL's is that they're actually executed when the
docshell initiates the URL load and thus the context they execute in is always
the one where the link came from.

We can fix part of this problem fairly easily, today the docshell always passes
'this' as the interface requestor to NS_OpenURI(). NS_OpenURI() is where the JS
in javascript: URL's is executed, passing in 'this' in the docshellcauses the JS
to always execute in the initiating window. In stead of passing in 'this' to
NS_OpenURI() we can do a FindItemWithName() in the dochshell and pass in the
other docshell as the interface requestor and the JS will be executed in the
correct window even if there's a target in the link *as long as the target
exists*. If the target window doesn't exist when the link is clicked
FindItemWithName() won't find the window so we'd haveto open the new window at
that time and that could have unknown bad side effects that I don't feel good
about at this stage in the game.

I'll attach a patch that makes targetting of javascript: URL's work as long as
the target window exists when the link is clicked. This is only a bandaid for
hiding the real problem...

Are there security issues we need to consider here, what if someone targets a
javascript: URL at a chrome window, will the normal security checks catch that
or would we need more code to deal with such a case?

Comments? Should we shoot for limbo with this?

Comment 29

[Johnny Stenback (:jst)]

Created attachment 18415 [details] [diff] [review]
Proposed fix that fixes JS URL's targetted at an existing window.

Comment 30

[Brendan Eich [:brendan]]

Cc'ing mstoltz to comment on the security hazards of your patch.

/be

Comment 31

[Johnny Stenback (:jst)]

I just spoke to Rick Potts about this and he thought it would be an acceptable
change for the branch but we decided to special case this for javascript: URL's
since nobody seems to really know exactly what the interface requestor that we
pass into NS_OpenURI() is used for so just to be on the safe side we should only
do the FindItemWithName() if a javascript: URL is loaded. I'll whip that change
into my patch...

Comment 32

[Johnny Stenback (:jst)]

Created attachment 18419 [details] [diff] [review]
New patch that only does a FindItemWithName() if a javascript: URL is loaded.

Comment 33

[Brendan Eich [:brendan]]

Something's wrong with those patches -- there's a reeeeeeeeeally long line of 
text above the added code, that's not a comment.  Also, the second patch looks 
just like the first?

/be

Comment 34

[hamerly]

Works for me as well.....using 10-31-00-14-MN6

Comment 35

[Johnny Stenback (:jst)]

Created attachment 18421 [details] [diff] [review]
New patch without the long unrelated line of text...

Comment 36

[Johnny Stenback (:jst)]

Oops, my bad, have a look at the latest attachment.

Comment 37

[Brendan Eich [:brendan]]

mstoltz: check me here, I think so long as the subject principals used to 
compile and execute (evaluate via an XPCOM proxied call) the script from the 
javascript: URI's path part come from the channel, never from the ifreq, jst's 
change is as secure as before, and fixes the problem that the script doesn't 
evaluate using the target window as scope chain.

Indeed, 
http://lxr.mozilla.org/mozilla/source/dom/src/jsurl/nsJSProtocolHandler.cpp#127 
gets the channel owner and QIs to nsIPrincipal.  So provided the channel for a 
javascript: URI always has as its owner the principal of the document in which 
the URI was embedded as a link (and of course that the QI succeeds), we are ok.

/be

Comment 38

[tompkins]

Downloaded 10/31 N6 latest build.  Works for me now, in latest build.

Comment 39

[Mitchell Stoltz (not reading bugmail)]

Brendan,
   Yes, this sounds all right to me. This change doesn't affect the assignment
of principals to the channel which happens in docshell.

Comment 40

[Brendan Eich [:brendan]]

jst: r=[e-mail] on your last patch, but keep a bug open on the horrid
strncasecmp that makes the fix a special case for javascript: URIs.  It should
be a general case, on the trunk, by mozilla0.9 I say.

/be

Comment 41

[Prashant Desale]

Adding keyword relnoteRTM, since there are lots of URL's with Target could be 
affected.

Comment 42

[Scott MacGregor]

I like jst's work around for now until Rick and I fix this for mozilla later on.
I'd also like to petition this for the rtm branch since so many commercial sites
use window targeting with JS urls.

sr=mscott

Comment 43

[Jason Eager]

This has a r= and sr=, and should go to rtm+ asap.

Comment 44

[Brendan Eich [:brendan]]

Ok, I'll set [rtm+] (hope I'm not stepping on anyone's toes).

This is yet another DOM level 0 compatibility bug that didn't get enough 
attention (my fault as much as anyone's; I took it from jst's list a while ago 
to "help", then got caught in "higher priority" stuff), and suddenly seemed to 
break important pages late in the game.  Those pages may seem to work without 
the fix, but this is yet another bug that could turn into a Big Compatibility 
Deal fast, next week or next month.

The fix helps greatly for frame targeting, and the only case it doesn't address 
is new-window.  I urge the pdt to take this bug for rtm.

/be

Comment 45

[selmer (gone)]

Please land it on the trunk and test with BuyerExpert 4.0.  Bring it back when 
we know how it performs.

Comment 46

[Johnny Stenback (:jst)]

The fix is checked into the trunk now.

Comment 47

[Brendan Eich [:brendan]]

jst, you are the man and I am not.  I'll do what I can to help get this rtm++'d 
and in Netscape 6.

/be

Comment 48

[Brendan Eich [:brendan]]

jar's radar, at least, ignores NEW bugs.  ASSIGNING for jst.

/be

Comment 49

[Jim Roskind]

We don't ignore "NEW bugs.
We do watch for "rtm+" bugs.
Please get info on buyer expert, add to bug, and renominate.
Thanks,
Jim

Comment 50

[Brendan Eich [:brendan]]

jar: ah, sorry -- thought I saw my name on your list (I got your mail), and I 
had no [rtm+] bugs (not this [rtm need info] one, which I reassigned to jst).

Buyer Expert 4.0 people have been contacted, looking for positive feedback.

/be

Comment 51

[tompkins]

Consider me the BuyerXpert 4.0 contact.  Our original problem related to using 
the javascript target field to rendor in another frame disappeared when we tried 
it on the 10/31 build.   From a BuyerXpert 4.0 perspective we consider our 
original problem (which we equated with this bug) to be resolved.

Comment 52

[Jason Eager]

So this should go back to rtm+ now?

Comment 53

[tompkins]

Tested on 11/02 (todays) build as well.  The reason I did this, is I'm 
not sure of your checkin/build policy/system, so am not sure where the 
fix is at the moment.  Is it being built, is it in 11/02 build, or?  
Anyway on 11/02 build, everything still aok for BuyerXpert 4.0 as it relates to 
this bug.

Comment 54

[Jason Eager]

This fix was placed in the trunk so it is in 11/02's build.

This has met all the criteria that the PDT asked for, and it should go back to rtm+.

Comment 55

[Scott MacGregor]

yes this should be rtm+. The limbo list is getting made right now. Hopefully we
didn't miss it. 

Comment 56

[selmer (gone)]

rtm-, already working for BuyerExpert 4.0 in 10/31 build.

Comment 57

[tompkins]

BuyerXpert 4.0 works with the branch bits as well (using the download link
below).  So our problem seems to be resolved.
ftp://sweetlou/products/client/seamonkey/windows/32bit/x86/2000-11-02-09-MN6
Mark

Comment 58

[WD]

Dupe of Bug 51237 ?
(Mozilla confuses multiple ports on same machine)

Comment 59

[Johnny Stenback (:jst)]

My workaround for this wasn't taken into the rtm branch so I'm reassigning this
to mscott who will fix this the right way on the trunk...

Comment 60

[tompkins]

Removing tompkins as cc

Comment 61

[ekrock's old account (dead)]

Nominating nsbeta1. DOM0 b.c. and we'd like the "fixed the right way" fix to be
included for nsbeta1.

Comment 62

[Dan Rosen]

what's up with this bug? looks like we have pantloads of ns6.0 rtm keywords...
is this bug fixed on the trunk? if so, might as well close it...

Comment 63

[scottputterman]

marking nsbeta1-

Comment 64

[Jesse Ruderman]

I think this patch introduced a cross-domain vulnerability: bug 77485, 
"function definition isn't subject to domain checks (exploit involves targeted 
javascript links)".

Comment 65

[eduardo]

another reference to see the problem:

http://www.severianoribeiro.com.br/

Comment 66

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Created attachment 140875 [details]
Testcase for the problem described in the summary

Comment 67

[Boris Zbarsky [:bzbarsky, bz on IRC]]

So what is the bug about at this point?  The testcase worksforme.  What's left
to do?

Comment 68

[Kurt Weinschenker]

testcase wfm with Camino nightly build 20040208, Mac OS 10.3.2.

Comment 69

[Brendan Eich [:brendan]]

Worksforme in 1.6 -- did something regress?

/be

Comment 70

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Huh?  What do you mean?  Nothing seems to have regressed that I can see... then
again, no one seems to be clear on what the right behavior is:

1)  Where should the JS be evaluated?
2)  With what priveleges?
3)  Where should the output be displayed?

At the moment, #3 is the target frame; #1 I'm not sure about (but it's easy to
test), #2 is the nsIPrincipal of the calling document.

Comment 71

[Brendan Eich [:brendan]]

bz: sorry, I thought someone was still claiming "broken".  If not, then
shouldn't this be WFM.  Any new problem goes in a new bug.

Regarding your questions:

1)  Where should the JS be evaluated?
2)  With what priveleges?
3)  Where should the output be displayed?

The answers are well-known and documented by various books, and more primarily,
by the MozillaClassic code.

The only wrinkle in the latest testcase not supported back in the old Netscape
Mozilla codebase is the data: URL, but a javascript: URL evaluating to a string
beginning with < would have the same effect (in that case, there would be an
inner javascript: URL in the a href attribute value).  The JS is evaluated when
the link is clicked, in the lower frame.  The privileges of generated documents
come from the generator.  The output goes in the link target.

Why is any of this unclear?  Really, I'm not being snarky -- can you point out
some conflicting spec or implementation?

/be

Comment 72

[Boris Zbarsky [:bzbarsky, bz on IRC]]

I wasn't saying that it was unclear what _should_ happen.  Just that it's
unclear to me what we do for #1.  And that's unclear because nsDocShell.cpp is a
bloody undocumented mess.

Comment 73

[Brendan Eich [:brendan]]

Yeah, docshell hurts my eyes.  I just reviewed a jst patch that had to touch it
(bug 68215).  It's in need of an enema before mozilla2.0, for sure.

/be