Jesse Ruderman Reporter
	Description • 19 years ago

The testcase causes my Mac trunk debug build to assert:

###!!! ASSERTION: Bound to wrong document: 'aDocument == GetCurrentDoc()', file /Users/admin/trunk/mozilla/content/base/src/nsGenericElement.cpp, line 1950

When this assertion fails, aDocument is non-null and GetCurrentDoc() is null.

	Jonas Sicking (:sicking) No longer reading bugmail consistently Assignee
	Comment 2 • 19 years ago

The fix here is to make us not fire scripts synchronously in BindToTree, right?

	Boris Zbarsky [:bzbarsky]
	Comment 3 • 18 years ago

Yes, exactly.  Then we can also remove the code that currently imperfectly works around this bustage in AppendChildTo, etc.

	Jonas Sicking (:sicking) No longer reading bugmail consistently Assignee
	Comment 4 • 18 years ago

This patch does a number of things:

1. The script element signals to the sink if it needs to block or not through
   a return value from the DoneAddingChildren function.
   This gets rid of the mNeedToBlockParser member.
2. Change the way ScriptAvailable notifications are done. Rather than *always*
   having to send out the notification, only send it out when we went into a
   blocking state. This together with 1 gets rid of the complicated and fragile
   reentry situation between nsScriptLoader::ProcessScriptElement and
   nsContentSink::ScriptAvailable. 
3. Break out common code from nsHTMLScriptElement and nsSVGScriptElement into
   a new parent class, nsScriptElement.
4. Decomtaminate nsScriptElement and nsScriptLoader (nsIScriptLoader is no more)
5. Move the logic in nsScriptLoader::OnStreamComplete into the new function
   nsScriptLoader::PrepareLoadedRequest so that it's easier to call
   nsScriptLoader::ProcessPendingRequests at all exit points.
6. Make the XML sink and the XSLT output handler use the same
   ns*ScriptElement::DoneAddingChildren function as the HTML sink rather than
   having to mess around with not inserting the script into the main DOM until
   it's fully parsed.
7. Add an |aNestingLevel| parameter to BeginUpdate and EndUpdate.
8. Hold off executing any script until EndUpdate has been called with a nesting
   level of 0.
9. Add ability for external code to tell the scriptloader to temporarily hold
   off executing any script.


The idea with using DoneAddingChildren is that it's a more generic method. Rather than the sink having to know which elements are scripts any element can just signal it back. We should eventually start calling DoneAddingChildren on *all* elements, which would simplify the sink and allow elements to hold off taking any action until the element is fully parsed.

There is one thing lacking in this patch I noticed. I was going to make it so that inserting an empty textnode as a child does not count as putting script in there. Only once there was some text inside the script would we execute it and set |mEvaluated|. This is why nsScriptElement observes CharacterDataChanged. I could add this to the patch as well, but it's already big as is. Let me know what you think.

I still need to test the XSLT changes, but the rest of the patch works fine.

	Jonas Sicking (:sicking) No longer reading bugmail consistently Assignee
	Comment 5 • 18 years ago

I tested the XSLT parts and it works fine

	Boris Zbarsky [:bzbarsky]
	Comment 6 • 18 years ago

Comment on attachment 241045 [details] [diff] [review]
Patch to fix

>Index: content/base/public/nsIContent.h

Need to rev the iid.

>+  virtual nsresult DoneAddingChildren(PRBool aHaveNotified)

I think this needs some documentation indicating what sort of nsresult values can be expected and/or what callers should do with them when they happen.  For example, not all error codes mean "abort and bail out" here, and I think this should be documented.

>Index: content/base/public/nsIDocument.h

Rev the iid.

>Index: content/base/public/nsIDocumentObserver.h

Rev the iid.

>   virtual void BeginUpdate(nsIDocument *aDocument,
>+                           nsUpdateType aUpdateType,
>+                           PRUint32 aNestingLevel) = 0;

Document this?  Especially the interaction (or lack thereof) between aNestingLevel and aUpdateType?

>+  virtual void EndUpdate(nsIDocument *aDocument,
>+                         nsUpdateType aUpdateType,
>+                         PRUint32 aNestingLevel) = 0;

Same.

>Index: content/base/public/nsINode.h
>+  nsSlots* GetSlots()

The only reaswon this became public is that we want to use it in the assertion in nsScriptElement::MaybeProcessScript, right?  How about we just add a debug-only inline method on nsINode for the purpose and keep GetSlots() private?  I don't see this assert as a good reason to make this public.

>+  PRBool HasFlag(PtrBits aFlag) const

And no one's using this at all; why make it public?

No matter what happens here, rev the iid.

>Index: content/base/public/nsIScriptElement.h

>+class nsIScriptElement : public nsIScriptLoaderObserver {

>+  PRUint32 GetScriptLineNumber()
>+  {
>+    return mLineNumber;
>+  }
>+  void SetIsMalformed()

I'd kinda like a newline separating the pairs of "related" methods.

>+  void SetDontExecute()
>+  {
>+    mIsEvaluated = PR_TRUE;
>+  }

Maybe call this PreventExecution() ?

>Index: content/base/public/nsIScriptLoaderObserver.idl

Rev the iid.

>Index: content/base/src/Makefile.in
>+		nsScriptLoader.h \

I'm really not all that happy with this, but I guess we used to export nsIScriptLoader, and random pieces of code like feeds depend on it.  I'd really prefer to have a stable API for disabling script exposed and stop exporting this; maybe followup bug?

>Index: content/base/src/nsContentSink.cpp
> nsContentSink::ScriptAvailable(nsresult aResult,

>+  NS_ASSERTION(mScriptElements.IndexOf(aElement) == count - 1 ||
>+               mScriptElements.IndexOf(aElement) == -1,
>+               "script found at unexpected position");

Document that index == -1 if the script was added via the DOM instead of via the parser?

>Index: content/base/src/nsScriptElement.cpp

Make sure to not check in the windows newlines in this file?

>+nsScriptElement::ScriptEvaluated(nsresult aResult,
>+{
>+    nsCOMPtr<nsPresContext> presContext;

So... This part is duplicated here and in ScriptAvailable.  Factor it out into a method?  Can even be a short static one here or something...

>+nsScriptElement::MaybeProcessScript()
>+    // The only error we don't ignore is NS_ERROR_HTMLPARSER_BLOCK

Document that there are success codes that need to be propagated out (like NS_CONTENT_SCRIPT_IS_EVENTHANDLER), which is why you're not just setting to NS_OK for anything that's not NS_ERROR_HTMLPARSER_BLOCK.

>Index: content/base/src/nsScriptElement.h

>+ * attributes and children of the class affects what script to execute

s/attributes/attribute/ and s/children/the children/ and s/affects/affect/.
And add a '.' at the end.  ;)

>+class nsScriptElement : public nsIScriptElement,

>+  // Internal methods

Should there be a "protected:" somewhere here?  I see no reason for MaybeProcessScript to be public...

>+   * that when adding a href attribute to an element that already contains an
>+   * inline script, the script referenced by the src attribute will not be
>+   * loaded.

s/href/src/

>+   * fallback-mechanism of using both inline script and linked script you have

Take out that dash between "fallback" and "mechanism".

>Index: content/base/src/nsScriptLoader.cpp

>+nsScriptLoader::nsScriptLoader(nsIDocument *aDocument)
>+  mDocument->AddObserver(this);

We need to RemoveObserver in ~nsScriptLoader if mDocument is non-null there, right?  Just in case the document outlives the loader?

> nsScriptLoader::~nsScriptLoader()

>+  for (PRInt32 i = 0; i < mPendingRequests.Count(); i++) {
>+    mPendingRequests[i]->FireScriptAvailable(NS_ERROR_ABORT);

So we're sure that the call to FireScriptAvailable won't change mPendingRequests?  If we want to play it safe, we could copy the array here, clear mPendingRequests, then enumerate the on-stack array...  And if we want to avoid the copy, we could use nsTArray<nsCOMPtr<nsScriptLoadRequest>> and use swap().  Again, only if FireScriptAvailable can change mPendingRequests.

>+nsScriptLoader::ProcessScriptElement(nsIScriptElement *aElement)

>-  if (!mEnabled || !mDocument->IsScriptEnabled() ||
>-      aElement->IsMalformed() || InNonScriptingContainer(aElement)) {

Can we at least assert !IsMalformed()?  Or not worth it?

>+  // XXX is this different from the mDocument->IsScriptEnabled() call?
>   if (globalObject)

Sadly, yes.  We have too many ways to disable script...  In particular nsDocument::IsScriptEnabled does an nsIScriptSecurityManager check (and will bail as "true" if it can't get a security manager).  That said, as long as we have a security manager the nsDocument::IsScriptEnabled check subsumes the script context one... so maybe we should remove this code in a followup bug?  I don't think we really support SSM-less operation anyway.

>@@ -578,172 +461,116 @@ nsScriptLoader::DoProcessScriptElement(n

>+    if (ReadyToExecuteScripts() && mPendingRequests.Count() == 0) {
>       request->mWasPending = PR_FALSE;

Shouldn't mWasPending be false here already?  If so, could we just assert it?

>+  NS_ENSURE_TRUE(mPendingRequests.AppendObject(request),
>+                 NS_ERROR_OUT_OF_MEMORY);

Should we cancel the channel, if any, if this fails?  Or not worry about it?  I guess if this fails we just won't execute the request?

> nsScriptLoader::FireScriptAvailable(nsresult aResult,
>-  PRInt32 count = mObservers.Count();
>-  for (PRInt32 i = 0; i < count; i++) {
>-    nsCOMPtr<nsIScriptLoaderObserver> observer = mObservers[i];

Again, we're sure the callee cannot remove the element from mObservers?  I wouldn't trust that, frankly...  That means you need to hold a strong ref to the observer outside of mObservers, otherwise it might die while you're calling into it, if it removes itself.

> nsScriptLoader::FireScriptEvaluated(nsresult aResult,

Same comment here.

> nsScriptLoader::OnStreamComplete(nsIStreamLoader* aLoader,

>+  if (NS_FAILED(rv)) {
>+    mPendingRequests.RemoveObject(request);
>+    FireScriptAvailable(NS_ERROR_NOT_AVAILABLE, request);

Why not |rv| instead of NS_ERROR_NOT_AVAILABLE?

>+nsScriptLoader::PrepareLoadedRequest(nsScriptLoadRequest* aRequest,

>+  NS_ASSERTION(mPendingRequests.IndexOf(aRequest) >= 0,
>+               "aRequest should be pending!");

Given that we don't handle failure to add to mPendingRequests by cancelling the loader, I don't think you can assert this.

>Index: content/base/src/nsScriptLoader.h

>+class nsScriptLoader : public nsIStreamLoaderObserver,

>+   * The loader maintains a strong reference to the document with

s/strong/weak/ ?

Can we get rid of DropDocumentReference and just use the DocumentWillBeDestroyed() notification we get as a document observer?  Either now, or in a followup bug.

>+  void AddObserver(nsIScriptLoaderObserver* aObserver)
>+  {
>+    mObservers.AppendObject(aObserver);

AppendObject can fail.  Should this have a way for caller to handle failure?

>+  nsresult ProcessScriptElement(nsIScriptElement* aElement);

This needs more documentation as to when it will or will not lead to ScriptAvailable and ScriptEvaluated being called...  If I understand right, the idea is that if this returns a failure that is not NS_ERROR_HTMLPARSER_BLOCK, they will not be called, right?  And otherwise they will?  In fact, the meaning of returning NS_ERROR_HTMLPARSER_BLOCK here should be well-documented, I think.

>+   * Whether the loader is enabled or not.
>+   * When disabled, processing of new script elements is disabled. 
>+   * Any call to processScriptElement() will fail with a return code of

s/processScriptElement/ProcessScriptElement/ ?

>+   * Add/remove blocker. Blockers will stop scripts from executing, but not
>+   * from loading.

This should document that removing the last blocker may synchronously execute the already-loaded scripts that were blocked from executing because we had a blocker in place.  To be precise, calling RemoveExecuteBlocker should not be done at "unsafe" times.

Now that I think on it, is processing script under EndUpdate() safe?  It seems to me that it is only if nsScriptLoader is notified after all the other document observers or something...  Otherwise we could call EndUpdate on some observers, then trigger more updates before the other observers get EndUpdate.

Maybe EndUpdate should return an nsIRunnable and nsDocument could stick those in an array or something as it calls EndUpdate on everyone... then run all the runnables?  Then we could just use an nsRunnableMethod here.

>Index: content/html/document/src/nsHTMLContentSink.cpp

> HTMLContentSink::ProcessSCRIPTEndTag(nsGenericHTMLElement *content,

>+  mCurrentContext->FlushTags(PR_FALSE);

This should be documented -- it's not obvious why this needs to be done.

>+      mScriptElements.AppendObject(sele);

Does anything interesting need to happen if this fails?  Won't we fail to unblock the parser in that case?

The cleanup in this function is most righteously excellent.

>Index: content/xml/document/src/nsXMLContentSink.cpp

>@@ -561,18 +557,40 @@ nsXMLContentSink::CloseElement(nsIConten
>+      // XXX The HTML sink doesn't call BlockParser here, why do we?

Good question.  File a followup bug to bring sanity to the parser-blocking API?

Same question here about appending the script to the script element array as for HTMLContentSink.

	Jonas Sicking (:sicking) No longer reading bugmail consistently Assignee
	Comment 7 • 18 years ago

> >+  virtual nsresult DoneAddingChildren(PRBool aHaveNotified)
> 
> I think this needs some documentation indicating what sort of nsresult values
> can be expected and/or what callers should do with them when they happen.  For
> example, not all error codes mean "abort and bail out" here, and I think this
> should be documented.

This one's a little bit tricky since the NS_ERROR_HTMLPARSER_BLOCK error code should be ignored by almost all callers. Ideally that code should not be an error code but rather a success value != NS_OK. But that's for a different bug.

For now I said that callers are free to recommend error codes and thus that implementations will have to deal with that.

> >Index: content/base/public/nsINode.h
> >+  nsSlots* GetSlots()
> 
> The only reaswon this became public is that we want to use it in the assertion
> in nsScriptElement::MaybeProcessScript, right?  How about we just add a
> debug-only inline method on nsINode for the purpose and keep GetSlots()
> private?  I don't see this assert as a good reason to make this public.

Agreed, created a #ifdef DEBUG function called DebugGetSlots().

> >+  PRBool HasFlag(PtrBits aFlag) const
> 
> And no one's using this at all; why make it public?

I think we should make this public anyway, but that can wait to a separate bug.

> >Index: content/base/src/Makefile.in
> >+		nsScriptLoader.h \
> 
> I'm really not all that happy with this, but I guess we used to export
> nsIScriptLoader, and random pieces of code like feeds depend on it.  I'd 
> really prefer to have a stable API for disabling script exposed and stop 
> exporting this; maybe followup bug?

Yeah, such an API should live elsewhere, no need to reach this far in with frozen APIs. At the very closest we should put it on nsIDocument, but not even that's frozen. Filed bug 358311

> > nsScriptLoader::~nsScriptLoader()
> 
> >+  for (PRInt32 i = 0; i < mPendingRequests.Count(); i++) {
> >+    mPendingRequests[i]->FireScriptAvailable(NS_ERROR_ABORT);
> 
> So we're sure that the call to FireScriptAvailable won't change
> mPendingRequests?  If we want to play it safe, we could copy the array here,
> clear mPendingRequests, then enumerate the on-stack array...  And if we want 
> to avoid the copy, we could use nsTArray<nsCOMPtr<nsScriptLoadRequest>> and 
> use swap().  Again, only if FireScriptAvailable can change mPendingRequests.

This is actually why i used mPendingRequests.Count() inside the for loop rather than to cache it in a local variable. If requests are added we probably want to abort them, and if they are removed (which I don't think can happen) then we probably don't want to abort them.

> >+nsScriptLoader::ProcessScriptElement(nsIScriptElement *aElement)
> 
> >-  if (!mEnabled || !mDocument->IsScriptEnabled() ||
> >-      aElement->IsMalformed() || InNonScriptingContainer(aElement)) {
> 
> Can we at least assert !IsMalformed()?  Or not worth it?

I don't care much either way. My idea was that things that are specific to the element the element should deal with. But assertions are good.

> >+  // XXX is this different from the mDocument->IsScriptEnabled() call?
> >   if (globalObject)
> 
> Sadly, yes.  We have too many ways to disable script...  In particular
> nsDocument::IsScriptEnabled does an nsIScriptSecurityManager check (and will
> bail as "true" if it can't get a security manager).  That said, as long as we
> have a security manager the nsDocument::IsScriptEnabled check subsumes the
> script context one... so maybe we should remove this code in a followup bug?  I
> don't think we really support SSM-less operation anyway.

I'll let you do that since I'm not sure what we want.

> >+  NS_ENSURE_TRUE(mPendingRequests.AppendObject(request),
> >+                 NS_ERROR_OUT_OF_MEMORY);
> 
> Should we cancel the channel, if any, if this fails?  Or not worry about it?  
> I guess if this fails we just won't execute the request?

Yeah, i think OOM is rare enough that we don't need to optimize it.

> > nsScriptLoader::FireScriptAvailable(nsresult aResult,
> >-  PRInt32 count = mObservers.Count();
> >-  for (PRInt32 i = 0; i < count; i++) {
> >-    nsCOMPtr<nsIScriptLoaderObserver> observer = mObservers[i];
> 
> Again, we're sure the callee cannot remove the element from mObservers?  I
> wouldn't trust that, frankly...  That means you need to hold a strong ref to
> the observer outside of mObservers, otherwise it might die while you're 
> calling into it, if it removes itself.

This should use nsTObserverArray and such. Filed bug 358317 and just added the strong ref for now.


> >+nsScriptLoader::PrepareLoadedRequest(nsScriptLoadRequest* aRequest,
> 
> >+  NS_ASSERTION(mPendingRequests.IndexOf(aRequest) >= 0,
> >+               "aRequest should be pending!");
> 
> Given that we don't handle failure to add to mPendingRequests by cancelling
> the loader, I don't think you can assert this.

I think I'd prefer to keep this assertion. If the request is not in that list it's much more likely there's a bug somewhere than that you ran out of memory in the exact right place. Especially if you get the assertion more than once.

> Can we get rid of DropDocumentReference and just use the
> DocumentWillBeDestroyed() notification we get as a document observer?  Either
> now, or in a followup bug.


We could get rid of it now. However this reminds me to remind you of something I asked you on irc. Rather than having the scriptloader be an nsIDocumentObserver, we could simply let nsDocument::BeginUpdate/EndUpdate call AddExecuteBlocker/RemoveExecuteBlocker. This would take less code I think (and is what I plan to do on branch to avoid modifying nsIDocumentObserver).

If we did that we'd have to keep DropDocumentReference.

> >+  nsresult ProcessScriptElement(nsIScriptElement* aElement);
> 
> This needs more documentation as to when it will or will not lead to
> ScriptAvailable and ScriptEvaluated being called...  If I understand right, the
> idea is that if this returns a failure that is not NS_ERROR_HTMLPARSER_BLOCK,
> they will not be called, right?  And otherwise they will?  In fact, the meaning
> of returning NS_ERROR_HTMLPARSER_BLOCK here should be well-documented, I think.

ScriptAvailable/ScriptEvaluated *will* only be called if NS_ERROR_HTMLPARSER_BLOCK is returned. If the script executes synchronously (i.e. is inline) the functions will already have been called by the time the function returns.

I'll document.

Btw, this means that the index in nsContentSink::ScriptAvailable is -1 for inline scripts.

> Now that I think on it, is processing script under EndUpdate() safe?  It seems
> to me that it is only if nsScriptLoader is notified after all the other
> document observers or something...  Otherwise we could call EndUpdate on some
> observers, then trigger more updates before the other observers get EndUpdate.
> 
> Maybe EndUpdate should return an nsIRunnable and nsDocument could stick those
> in an array or something as it calls EndUpdate on everyone... then run all the
> runnables?  Then we could just use an nsRunnableMethod here.

I've thought about this some too. I think we generally should be in a safe state when the last EndUpdate is called. If any of the EndUpdate implementations do something unsafe, that should be done once that EndUpdate function returns, right?

Aren't we mostly unsafe while nested inside C++ calls, but should be fine once those calls return?

I'm not entirely against the idea of having the scriptloader post nsIRunnables though, just in case. I think having EndUpdate returning them is a bit overengineered at this time.

> > HTMLContentSink::ProcessSCRIPTEndTag(nsGenericHTMLElement *content,
> >+  mCurrentContext->FlushTags(PR_FALSE);
> This should be documented -- it's not obvious why this needs to be done.

This was actually added due to a misunderstanding on my part. I thought that we called BeginUpdate and EndUpdate around the sink mutating the DOM. Turns out it doesn't, so it's not really needed. It still feels like the right thing to do though. Let me know what you think.

> >+      mScriptElements.AppendObject(sele);
> 
> Does anything interesting need to happen if this fails?  Won't we fail to
> unblock the parser in that case?

I guess we would :(
The alternative is to not block at all though, which equally will fail to render the page properly. Not really sure what to do (and I can't say that I'm super concerned given that if you're that out of memory most things will fail anyway).

> >Index: content/xml/document/src/nsXMLContentSink.cpp
> 
> >@@ -561,18 +557,40 @@ nsXMLContentSink::CloseElement(nsIConten
> >+      // XXX The HTML sink doesn't call BlockParser here, why do we?
> 
> Good question.  File a followup bug to bring sanity to the parser-blocking API?

Filed bug 358325. I also removed the extra |If (mParser)| check around the BlockParser() call.

	Jonas Sicking (:sicking) No longer reading bugmail consistently Assignee
	Comment 8 • 18 years ago

That should say "For now I said that callers are free to *ignore* error codes"

	Jonas Sicking (:sicking) No longer reading bugmail consistently Assignee
	Comment 9 • 18 years ago

This is the patch after those changes. Not requesting review since I'm sure I'll do more changes after some discussion.

	Jonas Sicking (:sicking) No longer reading bugmail consistently Assignee
	Comment 10 • 18 years ago

Actually, I made the switch to use nsTObserverArray since that's really simple. (who wrote that fabulous class anyway? ;) )

	Boris Zbarsky [:bzbarsky]
	Comment 12 • 18 years ago

> This one's a little bit tricky since the NS_ERROR_HTMLPARSER_BLOCK error code
> should be ignored by almost all callers.

Sure.  I think you should could just document that for script nodes this will return that code to indicate that the script has begun loading and will load asynchronously or something...  Either that, or just general muttering about how this error code indicates a pending async load of some resource and leave it at that?

Using the nsTObserverArray will make me happy in terms of my ownership concerns.    One issue with the last-posted patch -- it makes the nsScriptLoader no longer hold owning references to observers.  Is that desirable?

> I'll let you do that since I'm not sure what we want.

Thanks... I'll try to remember to do this...

> I think I'd prefer to keep this assertion.

Hmm... OK.  At least add documentation explaining that in this one edge case weird things can happen?

> we could simply let nsDocument::BeginUpdate/EndUpdate call
> AddExecuteBlocker/RemoveExecuteBlocker.

Hmmm....  You're right -- this might be simpler.  And wouldn't need a separate mInUpdate boolean.  Let'd do it.

> I think we generally should be in a safe state when the last EndUpdate is
> called.

Fair enough.

> It still feels like the right thing to do though. Let me know what you think.

If you can explain why you feel that way, I will... ;)  What's the point of doing the call there, exactly?  If the script touches the DOM in any way, we'll FlushTags at that point, no?

> The alternative is to not block at all though

Or to tell the scriptloader to lose the script from its pending scripts list.. I guess it doesn't much matter -- as you say the page will be hosed anyway.  I say document this as a design decision that uses less code.  ;)

	Jonas Sicking (:sicking) No longer reading bugmail consistently Assignee
	Comment 13 • 18 years ago

(In reply to comment #12)
> > This one's a little bit tricky since the NS_ERROR_HTMLPARSER_BLOCK error code
> > should be ignored by almost all callers.
> 
> Sure.  I think you should could just document that for script nodes this will
> return that code to indicate that the script has begun loading and will load
> asynchronously or something...  Either that, or just general muttering about
> how this error code indicates a pending async load of some resource and leave
> it at that?

Sounds good, I'll adjust the comment i put in there.

> Using the nsTObserverArray will make me happy in terms of my ownership
> concerns.    One issue with the last-posted patch -- it makes the
> nsScriptLoader no longer hold owning references to observers.  Is that
> desirable?

Doh! I'll leave nsTObserverArray for bug 358317.

> > I think I'd prefer to keep this assertion.
> 
> Hmm... OK.  At least add documentation explaining that in this one edge case
> weird things can happen?

Good idea, will do.

> > It still feels like the right thing to do though. Let me know what you think.
> 
> If you can explain why you feel that way, I will... ;)  What's the point of
> doing the call there, exactly?  If the script touches the DOM in any way, we'll
> FlushTags at that point, no?

We'll flush even before executing the script either way since the call to nsContentSink::ScriptAvailable will cause a flush call. This is the comment I had in the WIP patch:

// Flush all tags up front so that we are in as stable state as possible
// when calling DoneAddingChildren. This may not be strictly needed since
// any ScriptAvailable calls will cause us to flush anyway. But it gives a
// warm fuzzy feeling to be in a stable state before even attempting to
// run scripts.
// It would however be needed if we properly called BeginUpdate and
// EndUpdate while we were inserting stuff into the DOM.

> > The alternative is to not block at all though
> 
> Or to tell the scriptloader to lose the script from its pending scripts list..
> I guess it doesn't much matter -- as you say the page will be hosed anyway.  I
> say document this as a design decision that uses less code.  ;)

Hmm.. removing from the pending list is an interesting idea, i'll look into that.

	Boris Zbarsky [:bzbarsky]
	Comment 14 • 18 years ago

> This is the comment I had in the WIP patch:

That seems pretty good.  ;)

	Jonas Sicking (:sicking) No longer reading bugmail consistently Assignee
	Comment 15 • 18 years ago

This addresses all remaining comments.

	Boris Zbarsky [:bzbarsky]
	Comment 16 • 18 years ago

Comment on attachment 244650 [details] [diff] [review]
latest and greatest

Looks great!  r+sr=bzbarsky

	Jonas Sicking (:sicking) No longer reading bugmail consistently Assignee
	Comment 17 • 18 years ago

Checked in. Thanks for the review! Filed bug 359490 as followup to issue discussed on irc.

	Daniel Veditz [:dveditz]
	Comment 18 • 18 years ago

Did you want this on the 1.8 branch, or is bug 355221 sufficient?

	Jonas Sicking (:sicking) No longer reading bugmail consistently Assignee
	Comment 19 • 18 years ago

I think we should land this one as well, but i need to write a branch version.

	Jonas Sicking (:sicking) No longer reading bugmail consistently Assignee
	Comment 20 • 18 years ago

This one's good for branches. Should be a lot safer than the trunk one.

	Jonas Sicking (:sicking) No longer reading bugmail consistently Assignee
	Comment 21 • 18 years ago

This patch protects against a lot of different potential attack vectors where authors can change data structures in ways we don't expect and cause danling-pointer dereferences.

The branch patch is a lot safer than the trunk patch. None of changes in the branch patch caused any regressions (so far) when landed on trunk.

	Daniel Veditz [:dveditz]
	Comment 22 • 18 years ago

Comment on attachment 244969 [details] [diff] [review]
Branch patch

approved for 1.8/1.8.0 branches, a=dveditz for drivers

	Jonas Sicking (:sicking) No longer reading bugmail consistently Assignee
	Comment 23 • 18 years ago

Checked in on branches

	Bill Gianopoulos [:WG9s]
	Comment 24 • 18 years ago

This checkin caused the trunk regression bug 359888.

	Jesse Ruderman Reporter
	Comment 25 • 17 years ago

I checked in the testcase on this bug as a crashtest.  Feel free to create other tests; this was a large patch and the testcase tests stuff indirectly.