243392 - Execution of external javascript included by src don't stop

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Josef Sima]

User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1; .NET CLR 1.1.4322)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; cs-CZ; rv:1.6) Gecko/20040113

In the page at http://www.chose.cz/priklady/mozilla-external-js-bug/, are 
included some javascripts and if you click on some link while downloading (or 
requesting?) external script you will see result of included script on next 
page.
External javascript is simple php script that simulates slow request from 
server:
------
document.write('<p><i>Result from external javascript - timestamp <?php echo 
mktime(); ?></i></p>');
<?php sleep(1); ?>
------

Reproducible: Sometimes
Steps to Reproduce:
1. Include external javascript to page (<script 
src="http://path/to/javascript" type="text/javascript"></script>)
2. Press reload or click on some link on page when browser is downloading 
external javascript
Actual Results:  
On target page (or same page if you press reload) you will see result of 
external javascript.

Expected Results:  
Mozilla should have stop downloading and executing of javascript if user press 
reload or go to another page.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Worksforme with a current linux nightly -- I see no extra output on the second page.

Comment 2

[Brendan Eich [:brendan]]

$ wget -S http://www.chose.cz/priklady/mozilla-external-js-bug/javascript.php
--10:27:44--  http://www.chose.cz/priklady/mozilla-external-js-bug/javascript.php
           => `javascript.php.1'
Resolving www.chose.cz... done.
Connecting to www.chose.cz[81.0.234.33]:80... connected.
HTTP request sent, awaiting response...
 1 HTTP/1.1 200 OK
 2 Date: Wed, 12 May 2004 17:28:59 GMT
 3 Server: Apache/1.3.27 (Linux/SuSE) mod_xslt/1.0 mod_python/2.7.10
Python/2.2.2 PHP/4.3.1 mod_perl/1.27 mod_layout/3.0 mod_gzip/1.3.26.1a
 4 X-Powered-By: PHP/4.3.1
 5 Connection: close
 6 Content-Type: text/html
                                                                                
    [ <=>                                 ] 88            85.94K/s
                                                                                
10:27:45 (85.94 KB/s) - `javascript.php.1' saved [88]
                                                                                
Come on, your MIME type is text/html for those external .js files. And no way is
this a JavaScript Engine bug -- please file more cluefully after checking MIME
types next time.

/be

Comment 3

[Josef Sima]

Please look at http://be.seznam.cz/mozilla-bug/. Here is updated example with 
correct mime-types. 

To see the bug you must click on link during requesting of javascripts (text 
in status bar is "Wating for be.seznam.cz..."). On target page are two "hr" 
tags and between them you should have see text "Document.write from 
javascript.jss". This bug doesn't appear always, but only sometimes. If there 
isn't text, reload previous page and try it again.

Comment 4

[Brendan Eich [:brendan]]

Reopening, but this is *not* a JS engine bug.  htmlparser vs. document.write
code in content, maybe.

/be

Comment 5

[Boris Zbarsky [:bzbarsky]]

Josef, are you still testing 1.6?  If so, could you test a 1.7rc2 build, by any
chance?

Comment 6

[Josef Sima]

Yes. Now I have tested 1.7rc2 and still same :(

Comment 7

[Boris Zbarsky [:bzbarsky]]

OK, with that testcase I see the bug in a current Linux build.  Sicking, peterv,
any idea what's up here?  This is pretty bad.... (as in,
might-be-a-security-hole bad).

Comment 8

[Peter Van der Beken [:peterv]]

Hmm, I don't know a whole lot about the script loader. It looks as though we
don't call DropDocumentReference on the script loader when going to the next
page, which seems odd. Not sure if that's the problem though.

Comment 9

[Peter Van der Beken [:peterv]]

So it seems like this is what's happening: we cancel the loadgroup, we end up in
nsScriptLoader::OnStreamComplete with an error status, that makes us end up in
nsContentSink::ScriptAvailable and the content sink unblocks the parser and so
we insert another script element which kicks off a new script load.
We should probably disable the script loader before calling Cancel.

Comment 10

[Peter Van der Beken [:peterv]]

Attachment: Bandaid (breaks the fix for bug 98654)

I wanted to do this, it fixes the bug but breaks the fix for bug 98654. We
still need to figure out why the script gets executed in the new document.

Comment 11

[Peter Van der Beken [:peterv]]

Btw, I probably won't have a lot of time in the coming days to look at this.

Comment 12

[Peter Van der Beken [:peterv]]

Attachment: v1

Thanks to jst for the idea. This seems to fix it and doesn't break bug 98654.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Hmm.. can't you get an NS_BINDING_ABORTED from things other than document stop?
 I think it's the generic necko status for "something cancelled this"....

This patch may even work (based on my skim of uses of NS_BINDING_ABORTED in the
tree), but it seems fragile to me (could lead to documents being only partially
displayed if a script load returns NS_BINDING_ABORTED for some reason).

Comment 14

[Peter Van der Beken [:peterv]]

(In reply to comment #13)
> Hmm.. can't you get an NS_BINDING_ABORTED from things other than document stop?
>  I think it's the generic necko status for "something cancelled this"....

Could be, I haven't had time to really test this a lot. I guess the other
solution is to figure out a way to get to the parser from nsDocShell::Stop and
Terminate it from there.

Comment 15

[Boris Zbarsky [:bzbarsky]]

nsDocShell::Stop has a document, right?  So it could tell the document to
terminate the parser.....

Comment 16

[Peter Van der Beken [:peterv]]

It could tell a HTML document to terminate te parser, yes. None of the other
documents have access to their parser. We could move it to nsDocument I guess :-/.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Yeah, that may make sense....

Comment 18

[Peter Van der Beken [:peterv]]

Attachment: Alternative

This is a scary patch. We now call StopDocumentLoad twice, once from
nsDocShell::Stop and once from DocumentViewerImpl::Stop. And I see "Error
loading URL http://be.seznam.cz/mozilla-bug/index.html : 804b0002" in the
console. But hey, it fixes the bug.

Comment 19

[Boris Zbarsky [:bzbarsky]]

One other scary thing there -- reference cycles between XML
document/sink/parser...  Chances are, doing exactly what HTML did is enough, but
worth checking on...

Comment 20

[Peter Van der Beken [:peterv]]

I'll wait for darin to answer the question about NS_BINDING_ABORTED, I think he
comes back from holiday tomorrow.
I think the reference cycles are broken eventually, so it should be fine (famous
last words).

Comment 21

[Peter Van der Beken [:peterv]]

Here's what darin says:

It usually corresponds to the user pressing the STOP button in the browser.  In
other words, we try to make NS_BINDING_ABORTED be used only when some nsIRequest
failed due to user interaction.

I'd prefer to go with the first patch. Objections?

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 149540 [details] [diff] [review]
v1

darin, we have a lot of users of NS_BINDING_ABORTED in the tree that don't fit
your description.  See libpr0n if nothing else....  If this code has a meaning,
we need a different generic "just abort" kind of error code.

Peter, this is OK with me as long as you add a comment clearly explaining why
that check is being done and what we're assuming about NS_BINDING_ABORTED....

I still think the other patch represents a more sound overall approach, but I
agree that we wouldn't want to take it on the branch...

Comment 23

[Johnny Stenback (:jst)]

Comment on attachment 149540 [details] [diff] [review]
v1

sr=jst

Comment 24

[chris hofmann]

Comment on attachment 149540 [details] [diff] [review]
v1

ok, let's get this in 1.7 if it's ready-- a=chofmann

Comment 25

[Johnny Stenback (:jst)]

Fix checked in on the trunk and 1.7 branch.

Comment 26

[Peter Van der Beken [:peterv]]

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