178601 - document.write strangeness

Status: VERIFIED FIXED

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

Description

[Bob Clary [:bc] (inactive)]

I will attach 3 test cases which having varying behaviors under Mozilla 1.0.2
(Gecko/20021104) and Mozilla 1.2b (Gecko/20021104) in windows 2000.

test case 1: call an external script that defines a function that 
             obtains two references via document.getElementById then 
             does a document.write. Call the function at the end of
             the external script while the page is loading. 

             1.0.2/1.2b both load the test case without problem.

test case 2: same as test case 1 with the addition of a call to appendChild

             1.0.2 will clobber the page context replacing the page contents
             with the output of the document.write. Subsequent reloads will
             become 'stuck' in the wyciwyg protocol 

             1.2b will load the test case without problem

test case 3: an inline version of test case 2 where the script is executed 
             inline in the page in global scope.

             1.0.2 crash (TB 13568022, js_EmitTree 6ef98abc)
             1.2b  crash (TB 13568228, nsXPConnect::ReleaseJSContext 1af6fbb0)
      
this may be related to bug 177564

I am filing these together since it seems to me that the issues are related.
Note that bug 177564 occurs for both 1.0.2 and 1.2b.

Comment 1

[Bob Clary [:bc] (inactive)]

Attachment: test case 1 script

Comment 2

[Bob Clary [:bc] (inactive)]

Attachment: test case 1 html

Comment 3

[Bob Clary [:bc] (inactive)]

Attachment: test case 2 script

Comment 4

[Bob Clary [:bc] (inactive)]

Attachment: test case 2 html

Comment 5

[Bob Clary [:bc] (inactive)]

Attachment: test case 3 html (warning will crash 1.0.2 and 1.2b)

Comment 6

[Boris Zbarsky [:bzbarsky]]

So... the only bug still present in current builds is the crash, right?

Comment 7

[Bob Clary [:bc] (inactive)]

right.

Comment 8

[Bob Clary [:bc] (inactive)]

I should have mentioned that the 1.0.2 crash is immediate while the 1.2b case
locks up the browser and the crash occurs when you close the browser.

Comment 9

[Niklas Dougherty]

Test case 3 crashes 1.2b 2002103110 immediately on Mac OS X.

Comment 10

[Susie Wyshak]

Any known websites that crash due to this strangeness?

Comment 11

[Bob Clary [:bc] (inactive)]

None that I know of. 

These test cases were developed while trying to help an internal contact
reposition content generated from 3rd parties via document.write. 

The possible similarity in at least the 1.0.2 case to the mywashingtonpost.com
bug and the fact that continued manipulations of the content of a page
containing content inserted via document.write seemed to have problems both on
the branch and trunk lead me to believe that this might also shed light on the
mywashingtonpost.com bug.

Comment 12

[Harshal Pradhan]

Well, seems to me that at least for test case 3 the problem is a generic one and
has nothing to do with document.write per-se. 

Infact, I tried locally after removing just the document.write and my trunk
debug build still crashes. Its simply a case of uncontrolled recursion. 

The relevant fragment of markup is this:

<div id="d1">content before</div>
<div id="d2">
<script>
  var d1 = document.getElementById('d1');
  var d2 = document.getElementById('d2');
  d1.appendChild(d2);
</script>
</div>

So the script run by the <script> element is is basically appending the element
at some other point in the document. Adding a <script> element to a document
causes the script to run again, and thus this ends up executing itself again and
again. Eventually leading to crash due to out-of-memory or stack overflow. 

Stack : (this fragment repeats itself)

JS_EvaluateUCScriptForPrincipals(JSContext * 0x035c5138, JSObject * 0x03545160,
JSPrincipals * 0x03807980, const unsigned short * 0x03861a88, unsigned int 113,
const char * 0x00079eac, unsigned int 12, long * 0x00079d94) line 3382 + 25 bytes
nsJSContext::EvaluateString(nsJSContext * const 0x035c4f50, const nsAString &
{...}, void * 0x03545160, nsIPrincipal * 0x0380797c, const char * 0x00079eac,
unsigned int 12, const char * 0x00000000, nsAString & {...}, int * 0x00079df8)
line 701 + 85 bytes
nsScriptLoader::EvaluateScript(nsScriptLoadRequest * 0x03861410, const
nsAFlatString & {...}) line 585
nsScriptLoader::ProcessRequest(nsScriptLoadRequest * 0x03861410) line 492 + 22 bytes
nsScriptLoader::ProcessScriptElement(nsScriptLoader * const 0x038077b8,
nsIDOMHTMLScriptElement * 0x0383ae9c, nsIScriptLoaderObserver * 0x0383aea0) line
435 + 15 bytes
nsHTMLScriptElement::MaybeProcessScript() line 699
nsHTMLScriptElement::SetDocument(nsHTMLScriptElement * const 0x0383ae78,
nsIDocument * 0x03805628, int 1, int 1) line 499
nsGenericElement::SetDocumentInChildrenOf(nsIContent * 0x0383ab98, nsIDocument *
0x03805628, int 1) line 1808
nsGenericElement::SetDocument(nsGenericElement * const 0x0383ab98, nsIDocument *
0x03805628, int 1, int 1) line 1870 + 17 bytes
nsGenericHTMLElement::SetDocument(nsGenericHTMLElement * const 0x0383ab98,
nsIDocument * 0x03805628, int 1, int 1) line 1295 + 21 bytes
nsGenericHTMLContainerElement::InsertChildAt(nsGenericHTMLContainerElement *
const 0x0383a820, nsIContent * 0x0383ab98, int 1, int 1, int 1) line 3995
nsGenericElement::doInsertBefore(nsIDOMNode * 0x0383abbc, nsIDOMNode *
0x00000000, nsIDOMNode * * 0x0007a828) line 2819 + 35 bytes
nsGenericHTMLContainerElement::AppendChild(nsGenericHTMLContainerElement * const
0x0383a820, nsIDOMNode * 0x0383abbc, nsIDOMNode * * 0x0007a828) line 1069
nsHTMLDivElement::AppendChild(nsHTMLDivElement * const 0x0383a844, nsIDOMNode *
0x0383abbc, nsIDOMNode * * 0x0007a828) line 63 + 23 bytes
XPTC_InvokeByIndex(nsISupports * 0x0383a844, unsigned int 18, unsigned int 2,
nsXPTCVariant * 0x0007a818) line 106
XPCWrappedNative::CallMethod(XPCCallContext & {...}, XPCWrappedNative::CallMode
CALL_METHOD) line 2016 + 42 bytes
XPC_WN_CallMethod(JSContext * 0x035c5138, JSObject * 0x037d5230, unsigned int 1,
long * 0x0383bcf0, long * 0x0007aaf4) line 1294 + 14 bytes
js_Invoke(JSContext * 0x035c5138, unsigned int 1, unsigned int 0) line 839 + 23
bytes
js_Interpret(JSContext * 0x035c5138, long * 0x0007b9ac) line 2803 + 15 bytes
js_Execute(JSContext * 0x035c5138, JSObject * 0x03545160, JSScript * 0x03861f38,
JSStackFrame * 0x00000000, unsigned int 0, long * 0x0007b9ac) line 1020 + 13 bytes
JS_EvaluateUCScriptForPrincipals(JSContext * 0x035c5138, JSObject * 0x03545160,
JSPrincipals * 0x03807980, const unsigned short * 0x038610d0, unsigned int 113,
const char * 0x0007bac4, unsigned int 12, long * 0x0007b9ac) line 3382 + 25 bytes
nsJSContext::EvaluateString(nsJSContext * const 0x035c4f50, const nsAString &
{...}, v


There are a number of possible ways of fixing this, but I wonder which is the
most correct. The simplest probably would be a mEvaluating flag in the
nsHTMLScriptElement which would be checked and set just before a script
evaluation. (We already have a flag for "evaluated" but its useless in this
case.) Or possibly, this could be dealt with in the scriptloader. The patch from
bug 148277 would probably help in stopping the recursion and the crash.

If the 'flag in nsHTMLScriptElement' idea sounds good, I'd willingly do the patch. 

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

yes, an 'mEvaluating' flag sounds like the right fix to me.

Comment 14

[Harshal Pradhan]

Attachment: proposed fix

Well, this fixes the crash. (Also killed what looks like a pointless variable.)

However, I was wondering if setting the flag in ScriptAvailable was a better
idea.

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 107885 [details] [diff] [review]
proposed fix

I like approach best. mEvaluating mighe be a better name than mMaybeEvaluating,
but I can't really say i have a strong preference so feel free to leave it.

Comment 16

[Harshal Pradhan]

Attachment: variable renamed to mEvaluating

Comment 17

[Harshal Pradhan]

Comment on attachment 107887 [details] [diff] [review]
variable renamed to mEvaluating

marking flag for sicking's review.

Comment 18

[Harshal Pradhan]

Taking bug since I have a patch. 

Comment 19

[Asa Dotzler [:asa]]

Comment on attachment 107887 [details] [diff] [review]
variable renamed to mEvaluating

a=asa for checkin to 1.3a

Comment 20

[Harshal Pradhan]

Can someone check this in for me please. sicking, bz ?

Comment 21

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

checked in. Leaving for Harshal to mark fixed

Comment 22

[Harshal Pradhan]

All testcases in this bug now do the correct thing with current trunk.

Comment 23

[Bob Clary [:bc] (inactive)]

v 2002120908/win2k