36082 - Javascript from Superstats.com stops you from editing page.

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect, P3)

Description

[coffeewithmilk]

If you load a page in to the Editor/Composer which has the superstats banner,
and you are not connected to the internet, it will keep on trying to download
the extra bit from superstats, and since there is no button or menu to stop the
document loading, it will keep on going until you connect, this means you are
unable to edit the page, here is a sample of the offending code.

======================================================

<p><!-- Start SuperStats code version 2.0f.  Do not alter this code!
     http://www.superstats.com --><script language="JavaScript">
var code = " ";
</script>
<script src="http://code.superstats.com/code?u=aron">
</script>
<script language="JavaScript">
document.write(code);
</script>
<noscript><a href="http://stats.superstats.com/c.cgi?u=aron" target="_top"><img
SRC="http://stats.superstats.com/b.cgi?u=aron&z=1" BORDER=0 ></a></noscript><!--
End SuperStats tracking code. -->

Comment 1

[Jesse Ruderman]

What do you think the editor should do in this case?  Should it call the other 
file "code.js" and allow you to edit it alongside the main thing you're editing?

Comment 2

[rubydoo123]

assigning to sfraser -- this is probably related to your disable js in the 
editor bug.

Comment 3

[Simon Fraser [no longer active]]

actually, akkana owns the JS in editor bug.

Comment 4

[Akkana Peck]

It turns out that although the *execution* of JS is turned off for the editor,
that doesn't stop it from loading the *source* if a src= is specified in the JS
tag, and since that isn't found, the editor never gets the OnEndDocumentLoad it
needs to set itself up.  We'll need to figure out how to tell JS not to load its
src= URL if execution is suspended.

Comment 5

[rubydoo123]

setting to nsbeta3+

Comment 6

[rubydoo123]

adding the brackets in the status

Comment 7

[rubydoo123]

V

Comment 8

[rubydoo123]

adding Brendan, we may need your help and jbands help on this one

Comment 9

[Brendan Eich [:brendan]]

The mScriptsEnabled flag that akkana added to DOM script contexts apparently
must also affect the HTML content sink too, where script src= is processed, or
notifications don't work right.

Waitaminute -- how can !mScriptsEnabled break OnEndDocumentLoad notifications? 
Akkana, what do you mean by "and since that [src= URL] isn't found"?  Why is the
URL not found?  Is it dangling, in which case, we should be robust about such
things whether scripts are enabled or not?  Could there be some missing notify
call in the early-return cases predicated on !mScriptsEnabled in dom/src/base
nsJSEnvironment.cpp?

/be

Comment 10

[Akkana Peck]

My earlier suggestion was that if scripts were disabled, we shouldn't even
bother to load the source of the script, so the fact that we can't get to it
(because it's remote and our network is down) shouldn't come up at all.

I do agree that even in the scripts-enabled case we should probably be robust
about loading the page, and load the page without the script after the script
source times out (probably with some sort of error message in the status bar?) 
It sounds like we're not handling it that way now (and fixing that might be an
adequate temporary fix for this bug).  But in the script-disabled case, as in
the editor, the user shouldn't even have to wait for the netlib timeout, since
there's no need to get the script source at all.

Comment 11

[Brendan Eich [:brendan]]

Akkana: right, so can you check scriptContext->GetScriptsEnabled in the HTML
content sink (wish bugzilla didn't break long lines that contain links):

http://lxr.mozilla.org/mozilla/source/layout/html/document/src/nsHTMLContentSink.cpp#4807

You can see how to get the script context while in the content sink at

http://lxr.mozilla.org/mozilla/source/layout/html/document/src/nsHTMLContentSink.cpp#4505

/be

Comment 12

[Akkana Peck]

The fix Brendan suggested is very straightforward (thanks!) but I have no idea
if it works -- mozilla doesn't even launch when the network is disconnected. 
Added dependency on bug 7428.

Comment 13

[Brendan Eich [:brendan]]

Akkana, I'm dying to see a diff -u of the HTML content sink, just for closure
and to see that my talk wasn't cheap compared to the actual code.

/be

Comment 14

[Akkana Peck]

Attachment: Proposed patch (not tested yet)

Comment 15

[rubydoo123]

update status

Comment 16

[rubydoo123]

7428 has been minused, Akkana we either need to mark this minus and set to 
future, or you need to chek in the patch -- it's your call.

Comment 17

[Brendan Eich [:brendan]]

That patch looked good to me when it was attached -- let's get it in, ok?

/be

Comment 18

[Akkana Peck]

If Brendan likes the patch, I'm all for checking it in (though verifying the fix
might not be possible due to the other bug).  I've done so.

Comment 19

[sujay]

qa_contact over to tever, since 7428 is his bug also.

Comment 20

[sujay]

verified in 10/31 build.