javascript: URL links with TARGET not processed correctly.

RESOLVED WORKSFORME

Status

()

P2
normal
RESOLVED WORKSFORME
19 years ago
15 years ago

People

(Reporter: yamfe, Assigned: mscott)

Tracking

({dom0, helpwanted})

Trunk
Future
dom0, helpwanted
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [nsbeta2-][rtm-] r=brendan, sr=mscott, URL)

Attachments

(4 attachments)

(Reporter)

Description

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

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

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

19 years ago
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
Assignee: karnaze → norris

Comment 4

19 years ago
Change component to DOM and reassign.
Assignee: norris → vidur
Component: HTMLTables → DOM Level 0
QA Contact: chrisd → desale

Comment 5

19 years ago
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.
Whiteboard: 2 "simplest" testcases found

Comment 6

19 years ago
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.
Status: NEW → ASSIGNED
Summary: Additional text printed in table / backgroung colour in Frames → javascript: URL links with TARGET not processed correctly.
Target Milestone: M16

Comment 7

19 years ago
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 yamfe@yahoo.com) 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...
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.
Keywords: 4xp, nsbeta2

Comment 9

19 years ago
need to add a "missing image" bug to split off that problem. ekrock, can you do 
this?
Whiteboard: 2 "simplest" testcases found → [nsbeta2-]2 "simplest" testcases found

Comment 10

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

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

Comment 12

19 years ago
M16 has been out for a while now, these bugs target milestones need to be 
updated.
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.
Keywords: nsbeta3

Comment 14

18 years ago
Re-assigning to mscott for further analysis as per the decision in our bug 
triage meeting.
Assignee: vidur → mscott
Status: ASSIGNED → NEW
(Assignee)

Comment 15

18 years ago
*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. 
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+].
Priority: P3 → P2

Comment 17

18 years ago
comparing priority of this w/other mscott bugs...
Whiteboard: [nsbeta2-]2 "simplest" testcases found → [nsbeta2-][b3 need info]
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.
Keywords: helpwanted, rtm
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
Assignee: mscott → brendan

Updated

18 years ago
Status: NEW → ASSIGNED
Target Milestone: M16 → M18
(Assignee)

Comment 20

18 years ago
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.
Target Milestone: M18 → M16
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
(Assignee)

Comment 22

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

Updated

18 years ago
OS: Windows NT → All
Hardware: PC → All
Target Milestone: M16 → M18

Comment 23

18 years ago
B3 has passed.  Clearing "b3 need info"
Whiteboard: [nsbeta2-][b3 need info] → [nsbeta2-]

Updated

18 years ago
Target Milestone: M18 → ---

Comment 24

18 years ago
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.
Whiteboard: [nsbeta2-] → [nsbeta2-][need info]

Comment 25

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

18 years ago
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.
Tompkins, I followed your instructions and I do get the item in the left frame
when, am I missing something?
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?
Created attachment 18415 [details] [diff] [review]
Proposed fix that fixes JS URL's targetted at an existing window.
Cc'ing mstoltz to comment on the security hazards of your patch.

/be
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...
Created attachment 18419 [details] [diff] [review]
New patch that only does a FindItemWithName() if a javascript: URL is loaded.
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

18 years ago
Works for me as well.....using 10-31-00-14-MN6
Created attachment 18421 [details] [diff] [review]
New patch without the long unrelated line of text...
Oops, my bad, have a look at the latest attachment.
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

18 years ago
Downloaded 10/31 N6 latest build.  Works for me now, in latest build.
Brendan,
   Yes, this sounds all right to me. This change doesn't affect the assignment
of principals to the channel which happens in docshell.
jst: r=brendan@mozilla.org 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

18 years ago
Adding keyword relnoteRTM, since there are lots of URL's with Target could be 
affected.
Keywords: relnoteRTM
(Assignee)

Comment 42

18 years ago
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
Whiteboard: [nsbeta2-][need info] → [nsbeta2-][rtm need info]

Comment 43

18 years ago
This has a r= and sr=, and should go to rtm+ asap.
Whiteboard: [nsbeta2-][rtm need info] → [nsbeta2-][rtm need info] r=brendan, sr=mscott
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
Whiteboard: [nsbeta2-][rtm need info] r=brendan, sr=mscott → [nsbeta2-][rtm+] r=brendan, sr=mscott

Comment 45

18 years ago
Please land it on the trunk and test with BuyerExpert 4.0.  Bring it back when 
we know how it performs.
Whiteboard: [nsbeta2-][rtm+] r=brendan, sr=mscott → [nsbeta2-][rtm need info] r=brendan, sr=mscott
The fix is checked into the trunk now.
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
Assignee: brendan → jst
Status: ASSIGNED → NEW
jar's radar, at least, ignores NEW bugs.  ASSIGNING for jst.

/be
Status: NEW → ASSIGNED

Comment 49

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

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

18 years ago
So this should go back to rtm+ now?

Comment 53

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

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

Comment 55

18 years ago
yes this should be rtm+. The limbo list is getting made right now. Hopefully we
didn't miss it. 
Whiteboard: [nsbeta2-][rtm need info] r=brendan, sr=mscott → [nsbeta2-][rtm+] r=brendan, sr=mscott

Comment 56

18 years ago
rtm-, already working for BuyerExpert 4.0 in 10/31 build.
Whiteboard: [nsbeta2-][rtm+] r=brendan, sr=mscott → [nsbeta2-][rtm-] r=brendan, sr=mscott

Comment 57

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

18 years ago
Dupe of Bug 51237 ?
(Mozilla confuses multiple ports on same machine)
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...
Assignee: jst → mscott
Status: ASSIGNED → NEW

Comment 60

18 years ago
Removing tompkins as cc
Nominating nsbeta1. DOM0 b.c. and we'd like the "fixed the right way" fix to be
included for nsbeta1.
Keywords: nsbeta1
Keywords: dom0

Comment 62

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

18 years ago
marking nsbeta1-
Keywords: nsbeta1 → nsbeta1-
Target Milestone: --- → Future

Comment 64

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

15 years ago
another reference to see the problem:

http://www.severianoribeiro.com.br/
Created attachment 140875 [details]
Testcase for the problem described in the summary
So what is the bug about at this point?  The testcase worksforme.  What's left
to do?

Comment 68

15 years ago
testcase wfm with Camino nightly build 20040208, Mac OS 10.3.2.
Worksforme in 1.6 -- did something regress?

/be
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.
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
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → WORKSFORME
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.
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
You need to log in before you can comment on or make changes to this bug.