Closed Bug 130265 Opened 22 years ago Closed 21 years ago

onClick to change image source fails when href is "javascript:;"

Categories

(Core :: DOM: Navigation, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla1.4final

People

(Reporter: james_nospm, Assigned: jst)

References

()

Details

(Keywords: testcase, topembed+, Whiteboard: [adt1][HAVE FIX])

Attachments

(2 files, 2 obsolete files)

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 ( http://www.rot3k.com/3kingdoms/dw3/portraitsi.html ) 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 (
http://www.rot3k.com/3kingdoms/dw3/portraitsn.html ), 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: http://www.rot3k.com/3kingdoms/dw3/portraitsn.html
This fails: http://www.rot3k.com/3kingdoms/dw3/portraitsi.html

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.
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 -
Assignee: rogerl → pavlov
Status: UNCONFIRMED → NEW
Component: JavaScript Engine → ImageLib
Ever confirmed: true
OS: MacOS X → All
QA Contact: pschwartau → tpreston
Hardware: Macintosh → All
Compare bug 105257, "Image can't be changed in JS. Recent regression",
although that one is marked Verified Fixed...

cc'ing Boris -
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:

     http://www.rot3k.com/3kingdoms/dw3/portraitsi.html

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);
Changing the <img> SRC via the URL bar seems to work for me, as in:

javascript:
void(document['general'].src=http://www.mozilla.org/images/mozilla-banner.gif")
typo: missed the left double quote; should be this:

javascript:
void(document['general'].src="http://www.mozilla.org/images/mozilla-banner.gif")
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.
The other computers I tested on were running Mac OS 9.
Component: ImageLib → Image: Layout
"mass" re-assigning of bugs from pav to myself
Assignee: pavlov → jdunn
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.
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → INVALID
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.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
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).
Depends on: 83774
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?  
<shift> makes us not try to follow the link (since shift-click is "save link").
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
about:config
find 
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.

shouldn't the disk cache be used in that case (and re-decode the image of course)?
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.
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.
i.e.)
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.
Component: Image: Layout → JavaScript Engine
changing owner
Assignee: jdunn → pschwartau
Status: REOPENED → NEW
Um... how is loading behavior in any way, shape or form JS engine?  I'd think
that would be docshell, if anything...
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.
Assignee: pschwartau → adamlock
Component: JavaScript Engine → Embedding: Docshell
QA Contact: tpreston → adamlock
Attached file 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.
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.
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
killed?

Or do we just declare this site broken for not returning "false" from onclick?
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
though.
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!!
All,

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 Zeldman.com 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.
Well, if it's a widely used idiom, that makes it harder to just mark this
INVALID or WONTFIX.

What should the precedence be between the href and handler?
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?)
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
onclick.

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?

/be
brendan:

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.)
Depends on: 144587
Summary: onClick to change image source fails sometimes when onMouseOver works → onClick to change image source fails when href is "javascript:;"
This appears to create a problem for Netscape Radio. See internal reference:
http://bugscape.nscp.aoltw.net/show_bug.cgi?id=22057
Keywords: nsbeta1
adt: nsbeta1+/adt1
Keywords: nsbeta1nsbeta1+
Whiteboard: [adt1]
Attached patch Patch (obsolete) — Splinter Review
Is there anything wrong with just doing this?

Patch also removes some #include crap from nsWebShell.cpp
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.
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.
How hard is it to change things so that docshell evaluates the JS synchronously?

/be
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?
ADT: Nominating topembed
Keywords: topembed
darin: adamlock's patch is already adding a javascript: special case.  Why not
do the right thing, instead of the bogus strcmp?

/be
Keywords: testcase
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.
What the attached patch does *not* deal with is data: URL's that contain
JavaScript. Do we care?
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.
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.
What about wacky things such as js that changes window.location.href to point to
more js and things of that nature?
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.
Attachment #122782 - Flags: superreview?(darin)
Attachment #122782 - Flags: review?(adamlock)
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.

/be
Flags: blocking1.4?
Reassigning to Johnny since he's actively working on this.
Assignee: adamlock → jst
Target Milestone: --- → mozilla1.4final
Flags: blocking1.4? → blocking1.4+
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
Attachment #122782 - Flags: review?(adamlock) → review+
Blocks: 174579
*** Bug 192923 has been marked as a duplicate of this bug. ***
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));
>+    NS_ENSURE_TRUE(webNav, NS_ERROR_UNEXPECTED);
>+
>+    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).


> NS_IMETHODIMP
> 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.
Attachment #122782 - Flags: superreview?(darin) → superreview+
      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
branch).

/be
brendan:

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.
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.  

Keywords: topembedtopembed+
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.
Attachment #122308 - Attachment is obsolete: true
Attachment #122782 - Attachment is obsolete: true
Attachment #123145 - Flags: superreview?(darin)
Attachment #123145 - Flags: review?(adamlock)
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.)
Are the likes of mailto: also affected by this problem?
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! ;-)

sr=darin
Attachment #123145 - Flags: superreview?(darin) → superreview+
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.
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.
Status: NEW → ASSIGNED
Whiteboard: [adt1] → [adt1][HAVE FIX]
Comment on attachment 123145 [details] [diff] [review]
Same thing, but don't add nsJSChannel to the load group.

r=adamlock

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?
Attachment #123145 - Flags: superreview?(darin)
Attachment #123145 - Flags: superreview+
Attachment #123145 - Flags: review?(adamlock)
Attachment #123145 - Flags: review+
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 ;-)
Attachment #123145 - Flags: superreview?(darin) → superreview+
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!

/be
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.

/be
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 on attachment 123145 [details] [diff] [review]
Same thing, but don't add nsJSChannel to the load group.

Requesting approval for 1.4final.
Attachment #123145 - Flags: approval1.4?
Er, make that, assume it's not javascript if SchemeIs() fails, not return early.
(/me tries to stop his brain from spinning)
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
Attachment #123145 - Flags: approval1.4? → approval1.4+
FIXED.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago21 years ago
Resolution: --- → FIXED
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? 
this caused today's smoketest blocker: bug 205817
Depends on: 205817
Marking verified
Status: RESOLVED → VERIFIED
Keywords: verified1.4
*** Bug 144587 has been marked as a duplicate of this bug. ***
*** Bug 112398 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: