The default bug view has changed. See this FAQ.

document.write strangeness

VERIFIED FIXED

Status

()

Core
DOM: Core & HTML
--
critical
VERIFIED FIXED
15 years ago
9 years ago

People

(Reporter: bc, Assigned: Harshal Pradhan)

Tracking

({crash})

Trunk
x86
Windows 2000
crash
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 1 obsolete attachment)

(Reporter)

Description

15 years ago
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.
(Reporter)

Comment 1

15 years ago
Created attachment 105293 [details]
test case 1 script
(Reporter)

Comment 2

15 years ago
Created attachment 105294 [details]
test case 1 html
(Reporter)

Comment 3

15 years ago
Created attachment 105295 [details]
test case 2 script
(Reporter)

Comment 4

15 years ago
Created attachment 105296 [details]
test case 2 html
(Reporter)

Comment 5

15 years ago
Created attachment 105297 [details]
test case 3 html (warning will crash 1.0.2 and 1.2b)
So... the only bug still present in current builds is the crash, right?
(Reporter)

Comment 7

15 years ago
right.
Severity: normal → critical
Keywords: crash
(Reporter)

Comment 8

15 years ago
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

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

Comment 10

15 years ago
Any known websites that crash due to this strangeness?
(Reporter)

Comment 11

15 years ago
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.
(Assignee)

Comment 12

15 years ago
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. 
yes, an 'mEvaluating' flag sounds like the right fix to me.
(Assignee)

Comment 14

15 years ago
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.
(Assignee)

Updated

15 years ago
Attachment #107885 - Flags: review?(bugmail)
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.
Attachment #107885 - Flags: review?(bugmail) → review+
(Assignee)

Comment 16

15 years ago
Created attachment 107887 [details] [diff] [review]
variable renamed to mEvaluating
Attachment #107885 - Attachment is obsolete: true
(Assignee)

Comment 17

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

marking flag for sicking's review.
Attachment #107887 - Flags: review+
(Assignee)

Updated

15 years ago
Attachment #107887 - Flags: superreview?(bzbarsky)
(Assignee)

Comment 18

15 years ago
Taking bug since I have a patch. 
Assignee: jst → keeda
Attachment #107887 - Flags: superreview?(bzbarsky) → superreview+
(Assignee)

Updated

15 years ago
Attachment #107887 - Flags: approval1.3a?

Comment 19

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

a=asa for checkin to 1.3a
Attachment #107887 - Flags: approval1.3a? → approval1.3a+
(Assignee)

Comment 20

15 years ago
Can someone check this in for me please. sicking, bz ?
checked in. Leaving for Harshal to mark fixed
(Assignee)

Comment 22

15 years ago
All testcases in this bug now do the correct thing with current trunk.
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
(Reporter)

Comment 23

15 years ago
v 2002120908/win2k
Status: RESOLVED → VERIFIED

Updated

9 years ago
Component: DOM: Core → DOM: Core & HTML
QA Contact: stummala → general
You need to log in before you can comment on or make changes to this bug.