Persona is no longer an option for authentication on BMO. For more details see Persona Deprecated.
Last Comment Bug 130265 - onClick to change image source fails when href is "javascript:;"
: onClick to change image source fails when href is "javascript:;"
[adt1][HAVE FIX]
: testcase, topembed+
Product: Core
Classification: Components
Component: Document Navigation (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla1.4final
Assigned To: Johnny Stenback (:jst,
: Adam Lock
: 112398 144587 192923 (view as bug list)
Depends on: 83774 144587 205817
Blocks: 174579
  Show dependency treegraph
Reported: 2002-03-12 06:29 PST by James Peirce
Modified: 2011-08-05 21:34 PDT (History)
19 users (show)
asa: blocking1.4+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

simplified test case (723 bytes, text/html)
2003-04-10 18:13 PDT, Darin Fisher
no flags Details
Patch (2.31 KB, patch)
2003-05-02 10:53 PDT, Adam Lock
no flags Details | Diff | Splinter Review
Proposed fix, don't stop network trafic for javascript: URL's until we know we have to. (12.80 KB, patch)
2003-05-08 14:03 PDT, Johnny Stenback (:jst,
adamlock: review+
darin.moz: superreview+
Details | Diff | Splinter Review
Same thing, but don't add nsJSChannel to the load group. (13.35 KB, patch)
2003-05-13 09:51 PDT, Johnny Stenback (:jst,
adamlock: review+
darin.moz: superreview+
asa: approval1.4+
Details | Diff | Splinter Review

Description James Peirce 2002-03-12 06:29:29 PST
From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Macintosh; U; PPC Mac OS X; en-US; rv:0.9.9) Gecko/20020310
BuildID:    2002031005

On this page ( ) I use
onClick to make the image in the viewer (a table) to the right change.  When
loaded into Mozilla .99 (2002031005) anywhere from 20%-80% of the links fail to
work while some of the others work fine.  Each time I reload the page some of
the links that worked before fail and some that failed work.

When I go into the source and change the onClicks to onMouseOvers (and nothing
else) the whole page works again in Mozilla/Netscape.  Here is a page that
features the onMouseOvers instead of the onClicks ( ), which works just fine.

I have tested the JavaScript to death and gone out of my way to make sure it is
correct.  :(

This works:
This fails:

Reproducible: Always
Steps to Reproduce:
1. Set up a page with images and the javascript found on this page.
2. Load into Mozilla and it won't work.  I have tried with several differnt
pages now to see if the problem was something else, but I am afraid it is not.

Actual Results:  onClick sometimes fails to change the target image's source,
onMouseOver always works.

Expected Results:  onClick and onMouseOver should both cause the image to swap.
Comment 1 Phil Schwartau 2002-03-12 11:38:25 PST
Confirming bug with Mozilla trunk 20020311xx WinNT. 
OS: Mac OSX ---> All.

Just as James has reported, there is sporadic failure of the <img>
to update after you click on some link in the left-hand list.

The <img> element whose SRC attribute is being changed is:

<img src="../images/other/viewwindowtext.gif" name="general"
     class="inline" width="592" height="470">

The SRC attribute is changed via |document['general'].src =  (etc. etc.)
Stepping through the code in the JavaScript Debugger shows that this
assignment is successful. Indeed, click on any link to the left, and
enter this javascript: URL

             javascript: alert(document['general'].src);

You will see the correct path for the image every time. However, 
sporadically the image does not update visually; the previous image
remains in view and the new one is not seen. No errors occur in the
JavaScript Console. 

This is not a JavaScript Engine problem. Reassigning to ImageLib
for further analysis -
Comment 2 Phil Schwartau 2002-03-12 12:00:11 PST
Compare bug 105257, "Image can't be changed in JS. Recent regression",
although that one is marked Verified Fixed...

cc'ing Boris -
Comment 3 Phil Schwartau 2002-03-12 12:04:18 PST
As James has reported, the <img> swap works fine in the mouseover
example. The JavaScript looks identical in that case; it's just
fired from onMouseOver events instead of onClick events. 

It's only the onClick event example that (sporadically) fails:

Again, the following javascript: URL shows the DOM assignments to
the <img>'s SRC attribute are correctly being made on each click:

        javascript: alert(document['general'].src);
Comment 4 Phil Schwartau 2002-03-12 12:12:53 PST
Changing the <img> SRC via the URL bar seems to work for me, as in:

Comment 5 Phil Schwartau 2002-03-12 12:14:09 PST
typo: missed the left double quote; should be this:

Comment 6 James Peirce 2002-03-12 22:30:30 PST
I checked the page on several other machines using Mozilla .99.  The page worked fine on 
the initial load (although it did not on my own computer) and only displayed problems 
when I swapped to another image before the previous image has been displayed.  (For 
example, after the page loads I activated image swap A, but before image swap A's image 
had completely displayed I activated image swap B.  After displaying B image swap A 
would no longer work.

Any time I activated another swap before the previous one had displayed the link ceased to 
function.  When I exited the page, cleared the cache, and reloaded the page everything 
seemed to work again (until I duplicated the problem).

Also, if I tried to swap to an image that had not yet been preloaded it would do nothing 
and would continue to fail until cache had been cleared and the page reloaded.

This is a little different than the problem on my production computer (links just not 
working on initial load) but I thought I would share it anyway just in case I stumbled across 
something important.
Comment 7 James Peirce 2002-03-12 22:35:45 PST
The other computers I tested on were running Mac OS 9.
Comment 8 Jim Dunn 2003-02-04 06:46:46 PST
"mass" re-assigning of bugs from pav to myself
Comment 9 Jim Dunn 2003-02-21 07:21:46 PST
I am guessing this problem is NOT fixed, however just getting
to this I noticed that the page is no longer available.  Can
you retry with Mozilla 1.3b and if it is still happening, put
the site/testcase back up (or attach it to this bug)?

till then I am going to mark this as invalid, when the testcase
is available, just re-open.
Comment 10 James Peirce 2003-02-21 12:56:20 PST
Oh, sorry about that guys!
I took everything down when I moved the site quite some time ago.  I have posted
the test case again with the required script and images.  The rest of the page
images are missing, but it works in the other browsers again.  I will download
and test in Mozilla 1.3b tonight.
Comment 11 Boris Zbarsky [:bz] (still a bit busy) 2003-02-21 13:08:30 PST
This seems largely fixed by the patch in bug 83774 except for one thing --
images that have not yet loaded when the click comes in never seem to get
loaded.... (presumably because the load of "javascript:;" cancels the
in-progress document load).
Comment 12 Jim Dunn 2003-02-21 14:29:40 PST
btw in looking at this using mozilla 1.3b on win2k
I am seeing the problem HOWEVER if I do a <shift> mouse-click
it seems to load fine.  What does the <shift> give me?
refetching the image?  
Comment 13 Boris Zbarsky [:bz] (still a bit busy) 2003-02-21 17:16:57 PST
<shift> makes us not try to follow the link (since shift-click is "save link").
Comment 14 Jim Dunn 2003-04-03 14:36:28 PST
INTERESTING... this site preloads all of the images using newImage
but then later does a 
  document[changeImages.arguments[i]].src = changeImages.arguments[i+1];

This quickly fills up the memory cache (i.e. about:cache?device=memory
tells me:
    Maximum storage size: 	4194304 Bytes
    Storage in use:     	6058288 Bytes

To get around this... you can do
modify browser.cache.memory.capacity
click on the "value" field (i think default is 4096)
and change it to 35000.  

I found that the site when everything is loaded requires 33709k

NOTE: while the jpegs on the page are only 24k (or so)
we decode them and store these decoded images in memory
so with only 4k we overflow it very quickly and then
later when the imgCache goes to "get" them... nothing
is there.

Comment 15 Christian :Biesinger (don't email me, ping me on IRC) 2003-04-04 04:19:34 PST
shouldn't the disk cache be used in that case (and re-decode the image of course)?
Comment 16 Jim Dunn 2003-04-04 08:10:46 PST
I assume that is how it is suppose to work, but something must be
wrong.  Am starting to go through this now, figuring out when/where
decoded bits get written in, what checks get done, what happens
when cache is full...  I think this will fix a bunch of open bugs.
Comment 17 Jim Dunn 2003-04-10 13:00:16 PDT
Much of this bug is fixed.

What I found is that this now loads fine, UNLESS partway through
loading, you click on one of them, all loading stops and you
can't display ANY images that haven't been loaded.  I believe
(after talking Gordon) that this is probably how it is supposed
to work.
The current page is still loading when the user clicks a link.
The browser stops loading the current page and executes the handler 
for the link tag.  It happens simply change the current page.

and because the way the javascript is setup, clicking on a non
loaded image doesn't try to "get the image", just display it,
so if it hasn't been loaded, you don't see anything.

Am changing components... this probably should be closed as
won't fix or invalid at this point, but will let you decide.
Comment 18 Jim Dunn 2003-04-10 13:09:01 PDT
changing owner
Comment 19 Boris Zbarsky [:bz] (still a bit busy) 2003-04-10 16:10:43 PDT
Um... how is loading behavior in any way, shape or form JS engine?  I'd think
that would be docshell, if anything...
Comment 20 Phil Schwartau 2003-04-10 17:04:41 PDT
Following Boris' suggestion and reassigning to Docshell for a look
at the issue raised in Comment #6, Comment #11, and Comment #17.
The original problem seems to be fixed.
Comment 21 Darin Fisher 2003-04-10 18:13:05 PDT
Created attachment 120153 [details]
simplified test case

simplified test case.  summary of problem:

so there is a bug with onClick handlers that set src.  if the image is not
already in the image cache, then we'll display a broken image... or nothing at
all.  the link click is asynchronous, but LoadImage (from imageelement.src=foo)
happens synchronously.	as a result link click causes the initial LoadImage to
get canceled.  there are lines of html like this:  <a href="javascript:;"
onClick="..."> the onClick executes and then we follow the href, which kills
the load from the onClick handler.
Comment 22 gordon 2003-04-10 18:19:00 PDT
Phil, the original problem is *not* fixed.  In fact, it is worse than the
reporter realized, because actually both onClick and onMouseOver exhibit the
same problem, if you click on a link before the images are cached.  See the
simplified test case Darin just posted for the 'real' problem.
Comment 23 Boris Zbarsky [:bz] (still a bit busy) 2003-04-10 18:24:29 PDT
Can we somehow special-case Javascript urls here if they produce no output? 
Darin, we're basically killing off the old loadgroup on that javascript: load,
right?  So things like subframes that had not finished loading would also get

Or do we just declare this site broken for not returning "false" from onclick?
Comment 24 gordon 2003-04-11 13:04:15 PDT
Darin's test case has two simple work-arounds.  I think it may be reasonable to
mark this either INVALID or WONTFIX.  Using an empty javascript URL with an
onClick handler seems dubious.  I'd be interested in hearing other perspectives
Comment 25 Darin Fisher 2003-04-11 13:31:35 PDT
bz: yeah, i would think the link click would stop any pending network requests,
including those for subframes, etc.

brendan, jst:  i'm out of my element here.  please see bz's comment #23 and my
simplified testcase in comment #21.  thx!!
Comment 26 jeffm 2003-04-11 13:33:18 PDT

I know of many who use this quite extensively, indeed the famous "NYPL Style 
Guide" suggests this very approach (setting href="javascript:;" and using 
onclick). I first discovered the NYPL Style Guide through and I 
have seen portions of it quoted or referenced extensively around the web. (In 
other words, people may or may not be paying attention to this suggestion of 
the guide but I know that they do pay attention to the guide as a whole). 

Actually I've found that the bug also affects anchor elements with 
href="javascript:;" and have and event listener registered for click, instead 
of using onclick ( so addEventListener( 'click', func1, false )).

I don't have the time right now, but if needed I can provide test cases.
Comment 27 gordon 2003-04-11 13:39:30 PDT
Well, if it's a widely used idiom, that makes it harder to just mark this

What should the precedence be between the href and handler?
Comment 28 Jim Dunn 2003-04-11 14:15:13 PDT
Well Darin's testcase works the same in IE as it does in Mozilla.
However, he original website however is "different", it is ok in 
IE but fails in mozilla.

Per comment #26 : There is an open bug (bug 112398) that uses
an even listener to get javascript to change the image src.
(is this what you refer too?)
Comment 29 Brendan Eich [:brendan] 2003-04-11 14:25:29 PDT
Until you know you have a non-void result from a javascript: URL, you shouldn't
be canceling anything.  Or, if you have to cancel things when any link (of
whatever URI scheme) is clicked, you shouldn't be canceling loads started from

Is this hard to implement?  The first route sounds best, but it suggests the
easiest implementation would evaluate the JS expression synchronously.  We used
to do that long ago.  Can we go back?

Comment 30 Darin Fisher 2003-04-11 16:33:03 PDT

i mentioned to you that i thought link clicks were made asynchronous only
recently.  i was wrong about that.  in fact, i was remembering that clicks
resulting in a form submission were handled synchronously (bug 72906) as a
matter of helping to avoid double form submits.  so, we have indeed always (or
for a very long time) processed link clicks asynchronously.

as for evaluating the JS before handling the link click, i think that makes a
ton of sense.  i'm not exactly sure how to make things protocol agnostic at the
moment.  special casing javascript URLs seems mighty attractive, but maybe
something can be done.  (NOTE: the fact that we call nsILoadGroup::AddRequest
from nsIChannel::AsyncOpen makes it difficult to try to call AsyncOpen before
stopping any pending requests.)
Comment 31 Michael Dunn 2003-04-30 11:39:08 PDT
This appears to create a problem for Netscape Radio. See internal reference:
Comment 32 Samir Gehani 2003-04-30 16:18:45 PDT
adt: nsbeta1+/adt1
Comment 33 Adam Lock 2003-05-02 10:53:55 PDT
Created attachment 122308 [details] [diff] [review]

Is there anything wrong with just doing this?

Patch also removes some #include crap from nsWebShell.cpp
Comment 34 Heikki Toivonen (remove -bugzilla when emailing directly) 2003-05-02 12:05:07 PDT
Comment on attachment 122308 [details] [diff] [review]

This is no good. This would not fix , for example.
Comment 35 Boris Zbarsky [:bz] (still a bit busy) 2003-05-02 12:56:24 PDT
Um.  What if there is a space before the ';'?  Or after the ';'?  What if the
link is:

  <a href="javascript: alert('foo');">

?  It makes no sense to treat those differently.
Comment 36 Adam Lock 2003-05-02 13:32:56 PDT
The patch is not meant to be a thing of beauty but just to simply catch the most
common instance of this problem. The alternative is a far more extensive
rewiring of docshell so that it doesn't stop requests when clicking on a js
link. I'm not convinced that won't introduce its own problems.
Comment 37 Brendan Eich [:brendan] 2003-05-02 16:41:26 PDT
How hard is it to change things so that docshell evaluates the JS synchronously?

Comment 38 Darin Fisher 2003-05-02 16:56:17 PDT
brendan: that would mean special casing the link handler fu, which currently
processes the link click asynchronously.  also, you'd still have a race between
the onClick= and the href=.  to keep it so that javascript URLs always stop
existing browser activity, we'd have to ensure that href=javascript:... always
got executed before onClick=.  that seems difficult to ensure.

jst, heikki?
Comment 39 Paul Wyskoczka 2003-05-06 15:41:29 PDT
ADT: Nominating topembed
Comment 40 Brendan Eich [:brendan] 2003-05-06 16:46:44 PDT
darin: adamlock's patch is already adding a javascript: special case.  Why not
do the right thing, instead of the bogus strcmp?

Comment 41 Johnny Stenback (:jst, 2003-05-08 14:03:18 PDT
Created attachment 122782 [details] [diff] [review]
Proposed fix, don't stop network trafic for javascript: URL's until we know we have to.

This patch should be close to the right fix for this bug (and others). This
makes the docshell *not* stop current network activity when loading a
javascript: URL. In stead, the nsJSChannel will do that once it knows if the
evaluation of the javascript resulted in any data to parse.
Comment 42 Johnny Stenback (:jst, 2003-05-08 14:11:23 PDT
What the attached patch does *not* deal with is data: URL's that contain
JavaScript. Do we care?
Comment 43 Mike Shaver (:shaver -- probably not reading bugmail closely) 2003-05-08 14:15:56 PDT
Do we execute JS in data: URLs?  I (naively) expected not, because we don't seem
to execute JS that's loaded as a top-level document via an http: or file: URL.
Comment 44 Darin Fisher 2003-05-08 14:43:40 PDT
as for data: URLs... wouldn't a data: URL always result in a new document?  in
which case it seems appropriate for a link click to a data: URL to stop any and
all processing in the docshell.
Comment 45 Adam Lock 2003-05-08 15:08:06 PDT
What about wacky things such as js that changes window.location.href to point to
more js and things of that nature?
Comment 46 Johnny Stenback (:jst, 2003-05-08 15:16:01 PDT
Ah, correct you are (Mike and Darin). As for JS that changes the location or
document.writes() or anything like that, all that should be fine. If the
location is changed, or if document.write() is called, the code that runs as a
result of that stops all network traffic, and if none of that happens and it
doesn't return any data, we won't cancel, and that's what we want. And some data
is returned, then we'll stop whatever happened until that point. Also what we want.
Comment 47 Brendan Eich [:brendan] 2003-05-08 16:41:22 PDT
I think we want this for 1.4final, esp. if it's nsbeta1+ -- and whether or not
its patch fixes a regression from 1.0.

Comment 48 Heikki Toivonen (remove -bugzilla when emailing directly) 2003-05-08 17:12:45 PDT
Reassigning to Johnny since he's actively working on this.
Comment 49 Adam Lock 2003-05-09 07:13:56 PDT
Comment on attachment 122782 [details] [diff] [review]
Proposed fix, don't stop network trafic for javascript: URL's until we know we have to.

Looks fine, though NS_ENSURE_ARG_POINTER could be used for the aURI pointer
check in InternalLoad
Comment 50 Adam Lock 2003-05-09 11:54:27 PDT
*** Bug 192923 has been marked as a duplicate of this bug. ***
Comment 51 Darin Fisher 2003-05-09 19:24:58 PDT
Comment on attachment 122782 [details] [diff] [review]
Proposed fix, don't stop network trafic for javascript: URL's until we know we have to.

>Index: docshell/base/nsDocShell.cpp

>+    PRBool bIsJavascript = PR_FALSE;
>+    aURI->SchemeIs("javascript", &bIsJavascript);

remember aURI could be implemented outside our codebase, which means
that you should check the return value (i know you are just moving code,
and we probably have issues all over the place like this, but maybe a
good idea to clean them up as we encounter them).  something like this
would be better:

      PRBool bIsJavascript;
      rv = aURI->SchemeIs("javascript", &bIsJavascript);
      if (NS_FAILED(rv)) {
	  NS_WARNING("SchemeIs failed");
	  bIsJavascript = PR_FALSE; // assume not javascript and keep going.

>Index: dom/src/jsurl/nsJSProtocolHandler.cpp

>+nsresult nsJSChannel::StopAll()
>+    nsCOMPtr<nsIInterfaceRequestor> callbacks;
>+    mStreamChannel->GetNotificationCallbacks(getter_AddRefs(callbacks));
>+    nsCOMPtr<nsIWebNavigation> webNav(do_GetInterface(callbacks));
>+    return webNav->Stop(nsIWebNavigation::STOP_ALL);

footprint nit: single point of return will significantly reduce the
codesize of this function (NS_ENSURE_TRUE and final return statement
each adds code for 2 ~nsCOMPtr calls under GCC).

> nsJSChannel::Cancel(nsresult aStatus)
> {
>+    if (mIsActive && aStatus == NS_BINDING_ABORTED) {
>+        // We're aborted while active, this means we're canceled from
>+        // the call to StopAll() in Open() or AsyncOpen(), ignore the
>+        // cancel call, since we're not done with mStreamChannel yet.

i seem to remember a situation with bookmarklets in which cancel 
would get called while mIsActive is true.  we should do a lot of 
bookmarklet testing to make sure things are happy.

>+    // The javascript channel is considered the 'document channel',
>+    // but before we start loading the data from the stream, that
>+    // channel is canceled, so it's ok to pass that bit down...
>     //
>-    return mStreamChannel->SetLoadFlags(aLoadFlags & ~(LOAD_DOCUMENT_URI));
>+    return mStreamChannel->SetLoadFlags(aLoadFlags);

hmm... i think the problem is that AsyncOpen on mStreamChannel is called
before we remove |this| from the load group.  so, for a moment anyways
there will be two channels in the load group with LOAD_DOCUMENT_URI set.

doesn't the assertion at nsDocLoader.cpp:532 fire?

overall, this patch seems like the right solution to me.  sr=darin with
nits addressed.
Comment 52 Brendan Eich [:brendan] 2003-05-10 18:28:12 PDT
      PRBool bIsJavascript;
      rv = aURI->SchemeIs("javascript", &bIsJavascript);
      if (NS_FAILED(rv)) {
	  NS_WARNING("SchemeIs failed");
	  bIsJavascript = PR_FALSE; // assume not javascript and keep going.

Darin, why waste code footprint on the NS_FAILED test and branch?  The warning
is useless to end users of release builds, who won't see any warning; the result
is the same as jst's original code, which sets bIsJavaScript unconditionally (a
waste in the success case, true -- but small compared to an NS_FAILED test and

Comment 53 Darin Fisher 2003-05-11 12:59:17 PDT

well, i suppose it comes down to the rules of [xp]com... either,

  (1) the value of an out param cannot be trusted if the method returns a 
      failure code, or

  (2) an out param must not be assigned by a method unless it returns a 
      successful code.

which is correct?  i don't have a COM book handy, but i seem to recall that #1
is correct (it is certainly more consistent with the way we implement XPCOM
methods).  so, given that |SchemeIs| may be implemented by a drop-in XPCOM
component, do we really want to take chances?  i think it's worth the code bloat
to check return values, especially when potentially talking to foreign code.

the NS_WARNING suggestion is just about helping out component implementors...
probablly useless in this case, but since we are making an assumption about a
failure code meaning "not javascript" it seems like a good thing to flag in
debug code.
Comment 54 Doug Turner (:dougt) 2003-05-11 20:42:00 PDT
XPCOM doesn't explict state either, but rather XPCOM lets this question be
answered by the interface contract.  (1) is the most common practice.  

|SchemeIs| does not define any specific error codes.  It is possible that the
contract intended requires a boolean result.  Although I think that any XPCOM
method should be able to return an XPCOM error code without the interface
explictly mentioning these codes.  

Comment 55 Johnny Stenback (:jst, 2003-05-13 09:51:52 PDT
Created attachment 123145 [details] [diff] [review]
Same thing, but don't add nsJSChannel to the load group.

This is basically the same as above, but it makes sure the assertion darin
mentioned is never triggerd. When making this change, I was able to safely take
out the code that adds the nsJSChannel to the loadgroup (as suggested by darin)
and that ensured that the assertion darin mentioned is never triggerd since we
don't add anything to the loadgroup until we know that there's data to parse as
a result of evaluating the JS. With this change, there's no need to mask out
the LOAD_DOCUMENT_URI load flag here either since that flag will be set on the
stream channel that's used only when parsing data from a javascript: URL, and
in that case we always want that load flag set.
Comment 56 Boris Zbarsky [:bz] (still a bit busy) 2003-05-13 09:58:34 PDT
Darin, would it make sense to move the knowledge of "javascript: may not return
data" out of the docshell and into the protocol handler (adding a flag to
protocol handlers in general)?  Or are we not likely to hit any other such
pathological protocols?  (Doesn't affect the patch in this bug either way; this
would be 1.5-type work.)
Comment 57 Adam Lock 2003-05-13 12:28:09 PDT
Are the likes of mailto: also affected by this problem?
Comment 58 Darin Fisher 2003-05-13 13:03:21 PDT
Comment on attachment 123145 [details] [diff] [review]
Same thing, but don't add nsJSChannel to the load group.

ok, that bIsJavascript fu is real ooogly... and it still suffers from the same
problem at runtime (in release builds).  suppose SchemeIs sets retval to TRUE
and then returns a failure code?  that is possible though extrememly unlikely
in this case.  my point is just that we have to take care when talking to
foreign code.  i still think it should be a runtime check, but whatever! ;-)

Comment 59 Darin Fisher 2003-05-13 13:14:08 PDT
bz, adam: yeah, i talked to jst yesterday a little bit about that.  we decided
to keep it simple for now, but it seems like javascript:, mailto:, irc:, and
maybe even "unknown:" all have this behavior in common.  a flag on
nsIProtocolHandler might in fact be the right way to go.
Comment 60 Johnny Stenback (:jst, 2003-05-13 13:27:33 PDT
Yeah, I agree, but that's about as unlikely as someone simply just lying about
what scheme a URI is of with malicious intent. I.e. there's nothing that
guarantees that an implementation of nsIURI doesn't claim that it's an
javascript: URI even if it isn't. That's why I added the assertion (w/o messing
with rv) so that developers with good intentions are notified about this when
writing nsIURI implementations.

I'm ok with adding an early return there though (as we do when we check if the
URI is a wyciwyg URI), if others agree that it's worth the extra code size.

How about pushing the generalization of this into 1.5a though, I don't want to
invent new protocol handler flags now that we're past beta.
Comment 61 Adam Lock 2003-05-13 14:51:30 PDT
Comment on attachment 123145 [details] [diff] [review]
Same thing, but don't add nsJSChannel to the load group.


I'm not sure I like the schemeisrv being declared in the #ifdef DEBUG and used
outside of it as it is. Is SchemesIs() likely to fail anyhow?
Comment 62 Darin Fisher 2003-05-13 16:06:59 PDT
Comment on attachment 123145 [details] [diff] [review]
Same thing, but don't add nsJSChannel to the load group.

shaver made a good point (via email) that functions really shouldn't set out
params unless they are going to succeed.  otherwise we'd have problems in the
case of |out nsIFoo|.  of course, even that rule isn't strictly followed in the
codebase.  i guess it would be nice to say that there is a hard and fast rule
to follow in these cases, but it seems like it just comes down to the specifics
of particular methods.	in this case, we find it pathological for SchemeIs to
fail and set its out param to TRUE, so let's just keep things simple... i vote
for jst's original code...  sorry for all the useless raucous ;-)
Comment 63 Brendan Eich [:brendan] 2003-05-13 17:33:26 PDT
darin: you have shamed me for my closed-system thinking ;-).  I agree, test rv
and interpret the out param only on success.

bz (re: comment 56): don't be callin' my baby ugly now!

Comment 64 Brendan Eich [:brendan] 2003-05-13 17:37:01 PDT
Darn, I crossed streams with shaver -- armageddon is upon us for sure.

I'm indifferent.  Shaver's right about interface pointer out params, but boolean
and other scalars could be junk, in a pathological case (like, every other
plugin we've had to work around).  I defer to jst.

Comment 65 Johnny Stenback (:jst, 2003-05-13 18:48:20 PDT
I'll return early, given that this may have security consequences. If someone
thinks they have reasons to change this later, feel free to do so, but don't
bother asking for my oppinion :-)
Comment 66 Johnny Stenback (:jst, 2003-05-13 19:23:14 PDT
Comment on attachment 123145 [details] [diff] [review]
Same thing, but don't add nsJSChannel to the load group.

Requesting approval for 1.4final.
Comment 67 Johnny Stenback (:jst, 2003-05-13 22:52:17 PDT
Er, make that, assume it's not javascript if SchemeIs() fails, not return early.
(/me tries to stop his brain from spinning)
Comment 68 Asa Dotzler [:asa] 2003-05-14 14:24:16 PDT
Comment on attachment 123145 [details] [diff] [review]
Same thing, but don't add nsJSChannel to the load group.

a=asa (on behalf of drivers) for checkin to 1.4
Comment 69 Johnny Stenback (:jst, 2003-05-14 18:23:50 PDT
Comment 70 Phil Schwartau 2003-05-15 10:43:59 PDT
The given URL seems to work fine for me using today's
Mozilla trunk binary 2003051508 on WinNT.

James: does this work OK for you now, too? 
Comment 71 Peter Lubczynski 2003-05-15 11:47:23 PDT
this caused today's smoketest blocker: bug 205817
Comment 72 Paul Wyskoczka 2003-06-09 14:29:27 PDT
Marking verified
Comment 73 Johnny Stenback (:jst, 2003-06-10 09:59:25 PDT
*** Bug 144587 has been marked as a duplicate of this bug. ***
Comment 74 Boris Zbarsky [:bz] (still a bit busy) 2004-05-09 15:56:08 PDT
*** Bug 112398 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.