My testing indicates that's what other browsers (Safari, Opera, IE6/Windows, Konqueror) do. See testcases at http://web.mit.edu/bzbarsky/www/testcases/javascript-url/ (except dummy* and fail.html and pass.html). The patch coming up fixes for me bug 192128, bug 195562, bug 344874, bug 344882, bug 344996, bug 343940, bug 305546. It does not regress bug 343850 or bug 257875 or bug 139798.

Comment on attachment 237077 [details] [diff] [review] Fix (I've only read the -w patch. I trust you to get the indentation right :) ) + // we're just running under the event loop, so we shouldn't propagate JS + // exceptions out of here. + ::JS_ReportPendingException((JSContext*)scriptContext->GetNativeContext()); So, that's not quite true, since (chrome) JS can call open() on the channel, which calls this basically directly. Is that a problem? nsJSChannel::GetStatus(nsresult *aResult) { // We're always ok. Our status is independent of our underlying // stream's status. - *aResult = NS_OK; + *aResult = mStatus; Well that comment is lying now, isn't it :) nsJSChannel::Open(nsIInputStream **aResult) { - return InternalOpen(PR_FALSE, nsnull, nsnull, aResult); + NS_ENSURE_STATE(mIOThunk); How can mIOThunk be null here? + nsCOMPtr<nsIRunnable> ev = + new nsRunnableMethod<nsJSChannel>(this, &nsJSChannel::EvaluateScript); Need to nullcheck this. + if (NS_FAILED(rv)) { + loadGroup->RemoveRequest(this, nsnull, rv); + mIsActive = PR_FALSE; + } Should probably null out mContext here. + // Note that we want to be in the loadgroup when we do this so we find out + // if we get canceled. I don't understand this comment. Cancel() gets called if this channel gets cancelled. And, channels are supposed to be in their loadgroup all the time while they are pending... Is it just me or do you never set mOpenedStreamChannel to true? + // Otherwise, mStreamChannel will take over notifications. Perhaps this is slightly misleading, since it is the nsJSChannel which actually calls the listener's methods...

> So, that's not quite true, since (chrome) JS can call open() on the channel, Indeed. Modified comment accordingly. > Well that comment is lying now, isn't it :) Good catch. Fixed. ;) > How can mIOThunk be null here? I can't. Good catch, and fixed. > Need to nullcheck this. Actually, no. NS_DispatchToCurrentThread is documented to null-check stuff so callers don't have to worry about it. We use this pattern all over. > Should probably null out mContext here. Yep. And mListener. Fixed. > I don't understand this comment. I tried to make it clearer. Let me know if I did. > Is it just me or do you never set mOpenedStreamChannel to true? Good catch! Fixed. > Perhaps this is slightly misleading Indeed. I tried to make this clearer.

Comment on attachment 238957 [details] [diff] [review] Updated to sr comments + // script returns it). + + if (NS_SUCCEEDED(mStatus)) { please don't add trailing whitespace :) // Otherwise, mStreamChannel will call OnStartRequest and OnStopRequest on // us, so we'll be sure to call them on our listener. OK, but now the if above has both a then and an else-clause, so I'd just move the comment there and remove the "otherwise" :)

Comment on attachment 240861 [details] [diff] [review] Updated to tip Looks good to me, r=jst

Fixed on trunk.

could this have caused bug 356978 ?

Yeah, it did.

Could this have caused bug 365467?

Possible, sure.

Checked in mochitest for this.

(In reply to comment #16) > Checked in mochitest for this. some of these tests are failing on the linux tinderbox

we agreed to clobber the linux test box to reproduce this failure, but it didn't respond, so I'm leaving this checked-in. Surely someone will checkin to the closed tree with orange. ;)

Fixes the race condition that caused the orange.

(In reply to comment #19) > > Fixes the race condition that caused the orange. Still seeing one test fail.

About half the load-stopping tests are now commented out or otherwise not working. Setting back to in-testsuite? -- someone needs to figure out how to test this stuff from mochitest. I haven't been able to.