Closed Bug 130265 Opened 23 years ago Closed 22 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 ago22 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: