43652 - javascript: js in <a> tag clears window when executed

Status: VERIFIED FIXED

(Core :: Networking, defect, P1)

Description

[sean echevarria]

Clean build pulled 6/23 around 10:30am pacific.

Possibly a regression of http://bugzilla.mozilla.org/show_bug.cgi?id=34217.
Slightly different as the js DOES execute, but the window is cleared and the 
location gets set to the script that ran.

Comment 1

[sean echevarria]

nominating nsbeta2

Comment 2

[Andrew Kimpton]

Using "javascript:..." as the URL parameter to nsIPluginManager::GetURL() with 
a target of "_self" also shows the same behaviour. Setting 
the 'Force_Javascript' parameter to true doesn't fix the problem

Comment 3

[Johnny Stenback (:jst)]

Cc:ing valiski since his checkin on 06/16/2000 14:46 to mozilla/netwerk/base/
src/nsFileTransport.cpp, v1.79, broke the javascript: URL's. Also cc:ing warren
since he fixed this problem once before.
This simply *must* be fixed for beta2, period.

Comment 4

[Judson Valeski]

that change 
(http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&f
ile=nsFileTransport.cpp&root=/cvsroot&subdir=mozilla/netwerk/base/src&command=DI
FF_FRAMESET&rev1=1.78&rev2=1.79) was to fire OnStart() *always.* The consumer 
needs to accommodate the OnStart() that it wasn't getting before.

Comment 5

[Jay Patel [:jay]]

Putting on [nsbeta2+] radar for beta2 fix. 

Comment 6

[Michael Ward]

*** Bug 43708 has been marked as a duplicate of this bug. ***

Comment 7

[basic]

*** Bug 43708 has been marked as a duplicate of this bug. ***

Comment 8

[Gagan]

jud: who is the *consumer* here? the webshell? so who gets this bug? rpotts?

Comment 9

[sean echevarria]

*** Bug 44214 has been marked as a duplicate of this bug. ***

Comment 10

[Judson Valeski]

not sure actually, mscott maybe?

Comment 11

[Johnny Stenback (:jst)]

*** Bug 43851 has been marked as a duplicate of this bug. ***

Comment 12

[Johnny Stenback (:jst)]

nsDocumentOpenInfo::OnStartRequest() (in uriloader/base/nsURILoader.cpp) needs
to be able to get at the NS_ERROR_DOM_RETVAL_UNDEFINED error code returned from
nsJSThunk::Open() (in dom/src/jsurl/nsJSProtocolHandler.cpp), called from
nsFileTransport::Process(). If that's possible then we could check for that and
do nothin in OnStartRequest() in that case, this would fix this problem unless I
missed something here...

Comment 13

[Gagan]

->mscott

Comment 14

[Scott MacGregor]

Here's the problem:
1) the file channel does indeed set it's status correctly using
NS_ERROR_DOM_RETVAL_UNDEFINED.

2) However, the JS channel is a nsIOStreamChannel which wraps the file channel.
The uri loader only sees the JS channel.

3) Unfortunately, the status for the JS channel is not correctly set to
NS_ERROR_DOM_RETVAL_UNDEFINED. It's always NS_SUCCESS. So the uri loader doesn't
know to not propogate the OnStartRequest.

This appears to be a bug between the file transport setting the right code and
reflecting that back up in the channel that wraps the file transport
(nsIOStreamChannel). Who owns that?

Comment 15

[Scott MacGregor]

I think it would work if nsStreamIOChannel::GetStatus was modified to instead of
looking at it's own mStatus, return the status of the file transport instead.
But this breaks the case where mStatus for the io channel has a value.

Or maybe, if mStatus is NS_SUCCESS(mStatus) then return the status of the file
transport. That would preserve the semantics of mStatus while allowing us to get
at the value for the transport channel.

Here's the current version:
NS_IMETHODIMP
nsStreamIOChannel::GetStatus(nsresult *status)
{
    *status = mStatus;
    return NS_OK;
}

I'm arguing that we need
if (NS_SUCCEEDED(rv))
  *status = the status for mFileTransport
else
  *status = mStatus;

Warren, do you buy off on that?

Comment 16

[Scott MacGregor]

Attachment: proposed fix

Comment 17

[Scott MacGregor]

I re-implemented nsStreamIOChannel::GetStatus to return the status of the file
transport channel if it didn't have a non-error status code for its mStatus.

JS urls are now working again for me including this test case.

Warren, can you gimme a code review on the nsStreamIOChannel change?

Comment 18

[Warren Harris]

You should check the result value of aChannel->GetStatus(&rv) in 
nsURILoader.cpp just to be safe. Also, I think GetStatus should probably look 
like this:

  if (mFileTransport)
    rv = mFileTransport->GetStatus(status);
  else
    *status = mStatus;

Comment 19

[Scott MacGregor]

Hey Warren, I already added code to the uriloader to check for the status on the 
channel and to explicilty check for NS_ERROR_DOM_RETVAL_UNDEFINED. My problem 
was that the nsIOStreamChannel wasn't giving me this value. 

this change fixes the problem.

Can I consider this reviewed then? (including your suggestion) thanks man.

Comment 20

[Scott MacGregor]

Oh wait, Warren, the reason I didn't write the code the way you just did is 
because now you are losing mStatus if it was given an error code. If you search 
through the code in nsStreamIOchannel you'll see that in several cases mStatus 
is set with error codes (particularly in the cancel case). My patch works around 
that by preserving the value of mStatus and using it if it isn't a success 
value. By making your suggested change, we break the semantics of mStatus in 
nsStreamIOchannel and will incorrectly miss error values.

Comment 21

[Warren Harris]

I thought I saw that mStatus was only set by cancel. We could almost remove it 
entirely since canceling one of these without a transport is pretty meaningless 
(could return an error). But your patch is ok with me.

Comment 22

[Martin Horwath]

*** Bug 44767 has been marked as a duplicate of this bug. ***

Comment 23

[Scott MacGregor]

Fix checked in. This test js url is now working like it should for me. 

Comment 24

[R.K.Aa.]

*** Bug 44812 has been marked as a duplicate of this bug. ***

Comment 25

[R.K.Aa.]

*** Bug 45004 has been marked as a duplicate of this bug. ***

Comment 26

[Tom Everingham]

verif.
Win2000 2000071008
Linux 2000071008