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)
Core
DOM: Navigation
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)
723 bytes,
text/html
|
Details | |
13.35 KB,
patch
|
adamlock
:
review+
darin.moz
:
superreview+
asa
:
approval1.4+
|
Details | Diff | Splinter Review |
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.
Comment 1•23 years ago
|
||
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
Comment 2•23 years ago
|
||
Compare bug 105257, "Image can't be changed in JS. Recent regression",
although that one is marked Verified Fixed...
cc'ing Boris -
Comment 3•23 years ago
|
||
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);
Comment 4•23 years ago
|
||
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")
Comment 5•23 years ago
|
||
typo: missed the left double quote; should be this:
javascript:
void(document['general'].src="http://www.mozilla.org/images/mozilla-banner.gif")
Reporter | ||
Comment 6•23 years ago
|
||
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.
Reporter | ||
Comment 7•23 years ago
|
||
The other computers I tested on were running Mac OS 9.
Updated•23 years ago
|
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
Reporter | ||
Comment 10•22 years ago
|
||
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 → ---
Comment 11•22 years ago
|
||
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
Comment 12•22 years ago
|
||
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•22 years ago
|
||
<shift> makes us not try to follow the link (since shift-click is "save link").
Comment 14•22 years ago
|
||
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.
Comment 15•22 years ago
|
||
shouldn't the disk cache be used in that case (and re-decode the image of course)?
Comment 16•22 years ago
|
||
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•22 years ago
|
||
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
Comment 19•22 years ago
|
||
Um... how is loading behavior in any way, shape or form JS engine? I'd think
that would be docshell, if anything...
Comment 20•22 years ago
|
||
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
Comment 21•22 years ago
|
||
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•22 years ago
|
||
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•22 years ago
|
||
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?
Comment 24•22 years ago
|
||
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.
Comment 25•22 years ago
|
||
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•22 years ago
|
||
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.
Comment 27•22 years ago
|
||
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?
Comment 28•22 years ago
|
||
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•22 years ago
|
||
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
Comment 30•22 years ago
|
||
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.)
Updated•22 years ago
|
Summary: onClick to change image source fails sometimes when onMouseOver works → onClick to change image source fails when href is "javascript:;"
Comment 31•22 years ago
|
||
This appears to create a problem for Netscape Radio. See internal reference:
http://bugscape.nscp.aoltw.net/show_bug.cgi?id=22057
Keywords: nsbeta1
Comment 32•22 years ago
|
||
adt: nsbeta1+/adt1
Comment 33•22 years ago
|
||
Is there anything wrong with just doing this?
Patch also removes some #include crap from nsWebShell.cpp
Comment on attachment 122308 [details] [diff] [review]
Patch
This is no good. This would not fix
http://bugscape.nscp.aoltw.net/show_bug.cgi?id=22057 , for example.
Comment 35•22 years ago
|
||
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•22 years ago
|
||
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•22 years ago
|
||
How hard is it to change things so that docshell evaluates the JS synchronously?
/be
Comment 38•22 years ago
|
||
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 40•22 years ago
|
||
darin: adamlock's patch is already adding a javascript: special case. Why not
do the right thing, instead of the bogus strcmp?
/be
Assignee | ||
Comment 41•22 years ago
|
||
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.
Assignee | ||
Comment 42•22 years ago
|
||
What the attached patch does *not* deal with is data: URL's that contain
JavaScript. Do we care?
Comment 43•22 years ago
|
||
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•22 years ago
|
||
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•22 years ago
|
||
What about wacky things such as js that changes window.location.href to point to
more js and things of that nature?
Assignee | ||
Comment 46•22 years ago
|
||
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.
Assignee | ||
Updated•22 years ago
|
Attachment #122782 -
Flags: superreview?(darin)
Attachment #122782 -
Flags: review?(adamlock)
Comment 47•22 years ago
|
||
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
Updated•22 years ago
|
Flags: blocking1.4? → blocking1.4+
Comment 49•22 years ago
|
||
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+
Comment 50•22 years ago
|
||
*** Bug 192923 has been marked as a duplicate of this bug. ***
Comment 51•22 years ago
|
||
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+
Comment 52•22 years ago
|
||
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
Comment 53•22 years ago
|
||
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.
Comment 54•22 years ago
|
||
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.
Updated•22 years ago
|
Assignee | ||
Comment 55•22 years ago
|
||
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
Assignee | ||
Updated•22 years ago
|
Attachment #123145 -
Flags: superreview?(darin)
Attachment #123145 -
Flags: review?(adamlock)
Comment 56•22 years ago
|
||
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•22 years ago
|
||
Are the likes of mailto: also affected by this problem?
Comment 58•22 years ago
|
||
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+
Comment 59•22 years ago
|
||
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.
Assignee | ||
Comment 60•22 years ago
|
||
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 61•22 years ago
|
||
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 62•22 years ago
|
||
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+
Comment 63•22 years ago
|
||
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
Comment 64•22 years ago
|
||
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
Assignee | ||
Comment 65•22 years ago
|
||
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 :-)
Assignee | ||
Comment 66•22 years ago
|
||
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?
Assignee | ||
Comment 67•22 years ago
|
||
Er, make that, assume it's not javascript if SchemeIs() fails, not return early.
(/me tries to stop his brain from spinning)
Comment 68•22 years ago
|
||
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+
Assignee | ||
Comment 69•22 years ago
|
||
FIXED.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago → 22 years ago
Resolution: --- → FIXED
Comment 70•22 years ago
|
||
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?
Assignee | ||
Comment 73•21 years ago
|
||
*** Bug 144587 has been marked as a duplicate of this bug. ***
Comment 74•21 years ago
|
||
*** 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.
Description
•