131775 - [FIX]BeginUpdate is not always matched by EndUpdate in DOM code

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

We have a few places in our code where we BeginUpdate() but may not EndUpdate():

nsSVGAttributes::SetAttr
nsSVGAttributes::UnsetAttr
DOMMediaListImpl::BeginMediaChange/DOMMediaListImpl::EndMediaChange (the latter
   is not always called by the medialist manipulation methods)
DOMCSSDeclarationImpl::ParsePropertyValue
CSSStyleSheetImpl::InsertRule
CSSStyleSheetImpl::DeleteRule
CSSStyleSheetImpl::DeleteRuleFromGroup
CSSStyleSheetImpl::InsertRuleIntoGroup
nsGenericContainerElement::UnsetAttr
nsGenericContainerElement::SetAttr

Not assigning this to myself because I don't have time to do this till sometime
in mid-May and some of that code is hairy, but feel free to punt to me if this
is not a priority and if you're not going to get to it till then anyway, jst.

Comment 1

[Johnny Stenback (:jst)]

Mass-reassigning bugs to [e-mail]

Comment 2

[Boris Zbarsky [:bzbarsky]]

I guess I may as well take this....

Comment 3

[Johnny Stenback (:jst)]

Should we write a little nsAutoUpdate (or whatever) class to help here?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Yeah, that was what I was thinking...  I was gonna call it 
nsContentUpdateBatch, maybe... (or just nsUpdateBatch).

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch

This introduces the mozAutoDocUpdate class and uses it in various places.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Same as diff -w FOR REVIEW ONLY

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 132561 [details] [diff] [review]
Same as diff -w FOR REVIEW ONLY

reviews?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Incidentally, this fixes bug 211022 

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 132561 [details] [diff] [review]
Same as diff -w FOR REVIEW ONLY

This patch is bogus...

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch that's actually sorta correct

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Same as diff -w FOR REVIEW ONLY

Comment 12

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 132623 [details] [diff] [review]
Same as diff -w FOR REVIEW ONLY

General comment:

It seems there are quite a few callsites, (though most prominent in
nsSVGElement) where things are similar to:


  A::DoFoo()
  {
    mozAutoDocUpdate updateBatch(doc, UPDATE_CONTENT_MODEL, aNotify);
    if (foo) {
      // Do things which will send notifications
    }
  }

I think that in these cases, you can either use early returns if (!foo) since
nothing will happen to get rid of the extra scope, or move the mozAutoDocUpdate
inside the if so that we don't tell the document there will be udpates when
there really won't be.

There are cases in other places where you can't do an early return, so I would
just recommend changing the scope.  Let's send as few notifications to
observers as possible.	:-)



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

>   // Observation hooks used to propagate notifications to document observers.
>   // BeginUpdate must be called before any batch of modifications of the
>+  // content model or of style data, EndUpdate must be called afterward.  To
>+  // make this easy and painless, use the mozAutoDocUpdate helper class.  

Hm, I never thought I would make a nit this anal, but...

Could you move the "To" in the above comment down to the next line?  It seems
to dangle out there.  Sorry, I used to work at a newspaper a few years back and
that was one of the guidelines of making things more readable we tried to
follow (not ending a line with the first word of a multi-word statement when
said word is a short one), and I tend to agree that it makes it easier to read.



>   NS_IMETHOD BeginUpdate(nsUpdateType aUpdateType) = 0;
>   NS_IMETHOD EndUpdate(nsUpdateType aUpdateType) = 0;
>   NS_IMETHOD BeginLoad() = 0;
>   NS_IMETHOD EndLoad() = 0;
>   NS_IMETHOD ContentChanged(nsIContent* aContent,
>                             nsISupports* aSubContent) = 0;
>   // notify that one or two content nodes changed state
>   // either may be nsnull, but not both
>   NS_IMETHOD ContentStatesChanged(nsIContent* aContent1,
>                                   nsIContent* aContent2,
>@@ -467,20 +469,43 @@ public:
>                                const nsAString& Standalone) = 0;
>   NS_IMETHOD GetXMLDeclaration(nsAString& aVersion,
>                                nsAString& aEncoding,
>                                nsAString& Standalone) = 0;
> 
>   NS_IMETHOD_(PRBool) IsCaseSensitive() = 0;
> 
>   NS_IMETHOD_(PRBool) IsScriptEnabled() = 0;
> };
> 
>+class mozAutoDocUpdate {

Brace on its own line?	Also, could you please document we allow mDocument to
be null so that we don't have to null-check (and therefore add another scope)
in all the callers?

>+public:
>+  mozAutoDocUpdate(nsIDocument* aDocument, nsUpdateType aUpdateType,
>+                   PRBool aNotify) :
>+    mDocument(aNotify ? aDocument : nsnull),
>+    mUpdateType(aUpdateType)
>+  {
>+    if (mDocument) {
>+      mDocument->BeginUpdate(mUpdateType);
>+    }
>+  }
>+
>+  ~mozAutoDocUpdate()
>+  {
>+    if (mDocument) {
>+      mDocument->EndUpdate(mUpdateType);
>+    }
>+  }
>+
>+private:
>+  nsCOMPtr<nsIDocument> mDocument;
>+  nsUpdateType mUpdateType;
>+};
> 
> // XXX These belong somewhere else
> nsresult
> NS_NewHTMLDocument(nsIDocument** aInstancePtrResult);
> 
> nsresult
> NS_NewXMLDocument(nsIDocument** aInstancePtrResult);
> 
> #ifdef MOZ_SVG
> nsresult


>Index: content/base/src/nsGenericElement.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/base/src/nsGenericElement.cpp,v
>retrieving revision 3.291
>diff -p -u -1 -0 -w -r3.291 nsGenericElement.cpp
>--- content/base/src/nsGenericElement.cpp	2 Oct 2003 21:29:31 -0000	3.291
>+++ content/base/src/nsGenericElement.cpp	4 Oct 2003 08:23:41 -0000
>@@ -2606,23 +2606,24 @@ nsGenericElement::doInsertBefore(nsIDOMN
>     NS_ENSURE_TRUE(doc_fragment, NS_ERROR_UNEXPECTED);
> 
>     doc_fragment->DisconnectChildren();
> 
>     PRUint32 count = newContent->GetChildCount();
> 
>     PRUint32 old_count = GetChildCount();
> 
>     PRBool do_notify = !!aRefChild;
> 
>-    if (count && !do_notify && mDocument) {
>-      mDocument->BeginUpdate(UPDATE_CONTENT_MODEL);
>-    }    
>+    // If do_notify is true, then we don't have to handle the
>+    // notifications ourselves...  So pass !do_notify as aNotify.

I guess technically, you're only passing !do_notify if there are children. 
Also is the reader of the comment supposed to know where |aNotify| comes from?  

>+    mozAutoDocUpdate updateBatch(mDocument, UPDATE_CONTENT_MODEL,
>+                                 count && !do_notify);


>Index: content/html/style/src/nsCSSStyleSheet.cpp

>+#define BEGIN_MEDIA_CHANGE()                                   \
>+  if (mStyleSheet) {                                           \
>+    rv = mStyleSheet->GetOwningDocument(*getter_AddRefs(doc)); \
>+    NS_ENSURE_SUCCESS(rv, rv);                                 \
>+  }                                                            \
>+  mozAutoDocUpdate updateBatch(doc, UPDATE_STYLE, PR_TRUE);    \
>+  if (mStyleSheet) {                                           \
>+    rv = mStyleSheet->WillDirty();                             \
>+    NS_ENSURE_SUCCESS(rv, rv);                                 \
>+  }
>+
>+#define END_MEDIA_CHANGE()                                     \
>+  if (mStyleSheet) {                                           \
>+    mStyleSheet->DidDirty();                                   \
>+  }                                                            \
>+  /* XXXldb Pass something meaningful? */                      \
>+  if (doc) {                                                   \
>+    rv = doc->StyleRuleChanged(mStyleSheet, nsnull, nsnull);   \
>+    NS_ENSURE_SUCCESS(rv, rv);                                 \
>+  }
>+

How about making mStyleSheet and doc be parameters to the macros?  This way it
makes the call-sites more explicit as to what is going on, and doesn't rely on
a local being defined |doc| at each call site, though it doesn't really matter
much since you name the variable the same thing everywhere....	Does this also
need PR_BEGIN_MACRO and PR_END_MACRO?

Comment 13

[Christopher Aillon (sabbatical, not receiving bugmail)]

Oh duh, can't use PR_BEGIN/END_MACRO because of scoping.

Comment 14

[Boris Zbarsky [:bzbarsky]]

> though most prominent in nsSVGElement

There is only one callsite in nsSVGElement where I could change the scope (in
nsSVGElement::RemoveChildAt).  The rest are correct, in fact.

Added comments about mozAutoDocUpdate.

> Also is the reader of the comment supposed to know where |aNotify| comes from?  

Made comment clearer.

> How about making mStyleSheet and doc be parameters to the macros?

Good idea.  Done.

Comment 15

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 132623 [details] [diff] [review]
Same as diff -w FOR REVIEW ONLY

(btw, that was r= with comments fixed)

Comment 16

[Johnny Stenback (:jst)]

Comment on attachment 132623 [details] [diff] [review]
Same as diff -w FOR REVIEW ONLY

sr=jst

Comment 17

[Boris Zbarsky [:bzbarsky]]

Checked in, with caillon's review comments addressed.