Last Comment Bug 178601 - document.write strangeness
: document.write strangeness
Status: VERIFIED FIXED
: crash
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: x86 Windows 2000
: -- critical (vote)
: ---
Assigned To: Harshal Pradhan
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2002-11-05 22:57 PST by Bob Clary [:bc:]
Modified: 2008-07-31 02:34 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
test case 1 script (191 bytes, text/javascript)
2002-11-05 22:59 PST, Bob Clary [:bc:]
no flags Details
test case 1 html (391 bytes, text/html)
2002-11-05 23:01 PST, Bob Clary [:bc:]
no flags Details
test case 2 script (214 bytes, text/javascript)
2002-11-05 23:02 PST, Bob Clary [:bc:]
no flags Details
test case 2 html (389 bytes, text/html)
2002-11-05 23:03 PST, Bob Clary [:bc:]
no flags Details
test case 3 html (warning will crash 1.0.2 and 1.2b) (479 bytes, text/html)
2002-11-05 23:04 PST, Bob Clary [:bc:]
no flags Details
proposed fix (3.30 KB, patch)
2002-12-01 21:59 PST, Harshal Pradhan
jonas: review+
Details | Diff | Splinter Review
variable renamed to mEvaluating (3.27 KB, patch)
2002-12-01 23:15 PST, Harshal Pradhan
keeda: review+
bzbarsky: superreview+
asa: approval1.3a+
Details | Diff | Splinter Review

Description Bob Clary [:bc:] 2002-11-05 22:57:55 PST
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:] 2002-11-05 22:59:37 PST
Created attachment 105293 [details]
test case 1 script
Comment 2 Bob Clary [:bc:] 2002-11-05 23:01:14 PST
Created attachment 105294 [details]
test case 1 html
Comment 3 Bob Clary [:bc:] 2002-11-05 23:02:29 PST
Created attachment 105295 [details]
test case 2 script
Comment 4 Bob Clary [:bc:] 2002-11-05 23:03:32 PST
Created attachment 105296 [details]
test case 2 html
Comment 5 Bob Clary [:bc:] 2002-11-05 23:04:45 PST
Created attachment 105297 [details]
test case 3 html (warning will crash 1.0.2 and 1.2b)
Comment 6 Boris Zbarsky [:bz] (TPAC) 2002-11-05 23:21:02 PST
So... the only bug still present in current builds is the crash, right?
Comment 7 Bob Clary [:bc:] 2002-11-05 23:28:12 PST
right.
Comment 8 Bob Clary [:bc:] 2002-11-06 00:25:13 PST
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 2002-11-06 04:03:40 PST
Test case 3 crashes 1.2b 2002103110 immediately on Mac OS X.
Comment 10 Susie Wyshak 2002-11-08 08:56:06 PST
Any known websites that crash due to this strangeness?
Comment 11 Bob Clary [:bc:] 2002-11-08 09:04:22 PST
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 2002-11-29 07:52:34 PST
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 2002-11-29 15:22:22 PST
yes, an 'mEvaluating' flag sounds like the right fix to me.
Comment 14 Harshal Pradhan 2002-12-01 21:59:16 PST
Created attachment 107885 [details] [diff] [review]
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 2002-12-01 22:46:10 PST
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 2002-12-01 23:15:48 PST
Created attachment 107887 [details] [diff] [review]
variable renamed to mEvaluating
Comment 17 Harshal Pradhan 2002-12-01 23:17:33 PST
Comment on attachment 107887 [details] [diff] [review]
variable renamed to mEvaluating

marking flag for sicking's review.
Comment 18 Harshal Pradhan 2002-12-01 23:21:13 PST
Taking bug since I have a patch. 
Comment 19 Asa Dotzler [:asa] 2002-12-05 11:09:00 PST
Comment on attachment 107887 [details] [diff] [review]
variable renamed to mEvaluating

a=asa for checkin to 1.3a
Comment 20 Harshal Pradhan 2002-12-05 21:34:18 PST
Can someone check this in for me please. sicking, bz ?
Comment 21 Jonas Sicking (:sicking) No longer reading bugmail consistently 2002-12-06 00:17:43 PST
checked in. Leaving for Harshal to mark fixed
Comment 22 Harshal Pradhan 2002-12-08 06:16:03 PST
All testcases in this bug now do the correct thing with current trunk.
Comment 23 Bob Clary [:bc:] 2002-12-09 14:50:46 PST
v 2002120908/win2k

Note You need to log in before you can comment on or make changes to this bug.