319374 - XPath should ignore changes to anonymous content

Status: RESOLVED FIXED

(Core :: XML, defect)

Description

[timeless]

bz suggests looking at what nsContentList does.

basic steps:
1. setup a script that listens for a pageload like thing.
2. load a page which doesn't need scrollbars.
3. have the script use an xpath to get all //A anchors.
4. slowly iterate around them (use setTimeout/setInterval or something).
5. change the dimensions of the document such that you do need scrollbars.
6. at some point the iterator will throw an exception because the reflow of adding a scrollbar resulted in a change notification.

what follows is from the mozilla1.8 branch (where plugins suck and cause reflows when you blink at them), but note that the steps i'm describing above should be perfect for trunk.

00 transformiix!nsXPathResult::Invalidate(void)+0x4 [c:\build\chs3\build\mozilla\extensions\transformiix\source\xpath\nsxpathresult.cpp @ 296]
01 transformiix!nsXPathResult::AttributeChanged(class nsIDocument * aDocument = 0x00000002, class nsIContent * aContent = 0x0012e3a0, int aNameSpaceID = 0x8690fe, class nsIAtom * aAttribute = 0x00000002, int aModType = 0)+0x8 [c:\build\chs3\build\mozilla\extensions\transformiix\source\xpath\nsxpathresult.cpp @ 221]
02 gklayout!nsDocument::AttributeChanged(class nsIContent * aChild = 0x00000002, int aNameSpaceID = 0x12e3a0, class nsIAtom * aAttribute = 0x008690fe, int aModType = 2)+0x41 [c:\build\chs3\build\mozilla\content\base\src\nsdocument.cpp @ 2423]
03 gklayout!nsHTMLDocument::AttributeChanged(class nsIContent * aContent = 0x0a2b97a8, int aNameSpaceID = 0, class nsIAtom * aAttribute = 0x00aa65e0, int aModType = 1)+0xd6 [c:\build\chs3\build\mozilla\content\html\document\src\nshtmldocument.cpp @ 1204]
04 gklayout!nsXULElement::SetAttrAndNotify(int aNamespaceID = 0, class nsIAtom * aAttribute = 0x00aa65e0, class nsIAtom * aPrefix = 0x00000000, class nsAString_internal * aOldValue = 0x0012e4e0, class nsAttrValue * aParsedValue = 0x0012e590, int aModification = 0x1000001, int aFireMutation = 0, int aNotify = 1)+0x24c [c:\build\chs3\build\mozilla\content\xul\content\src\nsxulelement.cpp @ 1519]
05 gklayout!nsXULElement::SetAttr(int aNamespaceID = 0, class nsIAtom * aName = 0x00000000, class nsIAtom * aPrefix = 0x00000000, class nsAString_internal * aValue = 0x0012e6ec, int aNotify = 1)+0x1b3 [c:\build\chs3\build\mozilla\content\xul\content\src\nsxulelement.cpp @ 1440]
06 gklayout!nsXBLPrototypeBinding::AttributeChanged(class nsIAtom * aAttribute = 0x00aa65e0, int aNameSpaceID = 0, int aRemoveFlag = 0, class nsIContent * aChangedElement = 0x0a0a7528, class nsIContent * aAnonymousContent = 0x0a2a64d0, int aNotify = 1)+0x165 [c:\build\chs3\build\mozilla\content\xbl\src\nsxblprototypebinding.cpp @ 504]
07 gklayout!nsXBLBinding::AttributeChanged(class nsIAtom * aAttribute = 0x0012e924, int aNameSpaceID = 1, int aRemoveFlag = 0x10011, int aNotify = 0x3f)+0x2d [c:\build\chs3\build\mozilla\content\xbl\src\nsxblbinding.cpp @ 779]
08 gklayout!nsXULElement::SetAttrAndNotify(int aNamespaceID = 0, class nsIAtom * aAttribute = 0x00aa65e0, class nsIAtom * aPrefix = 0x00000000, class nsAString_internal * aOldValue = 0x0012e998, class nsAttrValue * aParsedValue = 0x0012ea48, int aModification = 0x1000001, int aFireMutation = 0, int aNotify = 1)+0xf3 [c:\build\chs3\build\mozilla\content\xul\content\src\nsxulelement.cpp @ 1485]
09 gklayout!nsXULElement::SetAttr(int aNamespaceID = 0, class nsIAtom * aName = 0x00000000, class nsIAtom * aPrefix = 0x00000000, class nsAString_internal * aValue = 0x0012ea70, int aNotify = 1)+0x1b3 [c:\build\chs3\build\mozilla\content\xul\content\src\nsxulelement.cpp @ 1440]
0a gklayout!nsGfxScrollFrameInner::SetAttribute(class nsIFrame * aBox = 0x0a712658, class nsIAtom * aAtom = 0x00aa65e0, int aSize = 0, int aReflow = 1)+0x61
0b gklayout!nsGfxScrollFrameInner::LayoutScrollbars(class nsBoxLayoutState * aState = 0x0012ec20, struct nsRect * aContentArea = 0x0012ec8c, struct nsRect * aOldScrollArea = 0x0012ebdc, struct nsRect * aScrollArea = 0x0012ec50)+0xcb [c:\build\chs3\build\mozilla\layout\generic\nsgfxscrollframe.cpp @ 2368]
0c gklayout!nsHTMLScrollFrame::Reflow(class nsPresContext * aPresContext = 0x0a1b8e10, struct nsHTMLReflowMetrics * aDesiredSize = 0x0012ee6c, struct nsHTMLReflowState * aReflowState = 0x0012ecfc, unsigned int * aStatus = 0x0012efb8)+0x337 [c:\build\chs3\build\mozilla\layout\generic\nsgfxscrollframe.cpp @ 818]
0d gklayout!nsContainerFrame::ReflowChild(class nsIFrame * aKidFrame = 0x0a17b16c, class nsPresContext * aPresContext = 0x0a1b8e10, struct nsHTMLReflowMetrics * aDesiredSize = 0x0012ee6c, struct nsHTMLReflowState * aReflowState = 0x0012ecfc, int aX = 0, int aY = 0, unsigned int aFlags = 0, unsigned int * aStatus = 0x0012efb8)+0x50 [c:\build\chs3\build\mozilla\layout\generic\nscontainerframe.cpp @ 904]
0e gklayout!ViewportFrame::Reflow(class nsPresContext * aPresContext = 0x0a1b8e10, struct nsHTMLReflowMetrics * aDesiredSize = 0x0012eff0, struct nsHTMLReflowState * aReflowState = 0x0012ef00, unsigned int * aStatus = 0x0012efb8)+0x9d [c:\build\chs3\build\mozilla\layout\generic\nsviewportframe.cpp @ 240]
0f gklayout!IncrementalReflow::Dispatch(class nsPresContext * aPresContext = 0x0a17afa0, struct nsHTMLReflowMetrics * aDesiredSize = 0x09a3d240, struct nsSize * aMaxSize = 0x0012f06c, class nsIRenderingContext * aRendContext = 0x0a5bf528)+0xa6 [c:\build\chs3\build\mozilla\layout\base\nspresshell.cpp @ 914]
10 gklayout!PresShell::ProcessReflowCommands(int aInterruptible = 0)+0x129 [c:\build\chs3\build\mozilla\layout\base\nspresshell.cpp @ 6870]
11 gklayout!PresShell::FlushPendingNotifications(mozFlushType aType = Flush_Layout (0xf))+0x49 [c:\build\chs3\build\mozilla\layout\base\nspresshell.cpp @ 5333]
12 gklayout!nsDocument::FlushPendingNotifications(mozFlushType aType = Flush_Layout (0xf))+0x113 [c:\build\chs3\build\mozilla\content\base\src\nsdocument.cpp @ 4253]
13 gklayout!nsHTMLDocument::FlushPendingNotifications(mozFlushType aType = Flush_Layout (0xf))+0x59 [c:\build\chs3\build\mozilla\content\html\document\src\nshtmldocument.cpp @ 1220]
14 gklayout!nsHTMLExternalObjSH::GetPluginInstance(class nsIXPConnectWrappedNative * wrapper = 0x0a7247e8, class nsIPluginInstance ** _result = 0x0a476d90)+0x50 [c:\build\chs3\build\mozilla\dom\src\base\nsdomclassinfo.cpp @ 8358]
15 gklayout!nsHTMLExternalObjSH::PostCreate(class nsIXPConnectWrappedNative * wrapper = 0x0a0d5780, struct JSContext * cx = 0x00aef8a0, struct JSObject * obj = 0x05aaa288)+0x40 [c:\build\chs3\build\mozilla\dom\src\base\nsdomclassinfo.cpp @ 8418]
16 xpc3250!XPCWrappedNative::GetNewOrUsed(class XPCCallContext * ccx = 0x0012f40c, class nsISupports * Object = 0x054e3ed8, class XPCWrappedNativeScope * Scope = 0x09a1da00, class XPCNativeInterface * Interface = 0x015c73a8, class XPCWrappedNative ** resultWrapper = 0x0012f204)+0x3ce [c:\build\chs3\build\mozilla\js\src\xpconnect\src\xpcwrappednative.cpp @ 456]
17 xpc3250!XPCConvert::NativeInterface2JSObject(class XPCCallContext * ccx = 0x0012f40c, class nsIXPConnectJSObjectHolder ** dest = 0x0012f240, class nsISupports * src = 0x0a724800, struct nsID * iid = 0x0012f384, struct JSObject * scope = 0x05aaa270, int allowNativeWrapper = 1, unsigned int * pErr = 0x0012f3d0)+0x79 [c:\build\chs3\build\mozilla\js\src\xpconnect\src\xpcconvert.cpp @ 1107]
18 xpc3250!XPCConvert::NativeData2JS(class XPCCallContext * ccx = 0x00000000, long * d = 0x0012f3e4, void * s = 0x0012f268, class nsXPTType * type = 0x0012f3e0, struct nsID * iid = 0x0012f384, struct JSObject * scope = 0x05aaa270, unsigned int * pErr = 0x0012f3d0)+0x2fe [c:\build\chs3\build\mozilla\js\src\xpconnect\src\xpcconvert.cpp @ 468]
19 xpc3250!XPCWrappedNative::CallMethod(class XPCCallContext * ccx = 0x0012f40c, XPCWrappedNative::CallMode mode = CALL_METHOD (0))+0x85a [c:\build\chs3\build\mozilla\js\src\xpconnect\src\xpcwrappednative.cpp @ 2228]
1a xpc3250!XPC_WN_CallMethod(struct JSContext * cx = 0x00aef8a0, struct JSObject * obj = 0x05aaa270, unsigned int argc = 0, long * argv = 0x021b66e4, long * vp = 0x0012f4cc)+0x8e [c:\build\chs3\build\mozilla\js\src\xpconnect\src\xpcwrappednativejsops.cpp @ 1444]
1b js3250!js_Invoke(struct JSContext * cx = 0x00000001, unsigned int argc = 0, unsigned int flags = 0)+0x556 [c:\build\chs3\build\mozilla\js\src\jsinterp.c @ 1177]
1c js3250!js_Interpret(struct JSContext * cx = 0x00aef8a0, unsigned char * pc = 0x026082ac ":", long * result = 0x0012f754)+0x4fb5 [c:\build\chs3\build\mozilla\js\src\jsinterp.c @ 3524]
1d js3250!js_Invoke(struct JSContext * cx = 0x00000001, unsigned int argc = 2, unsigned int flags = 0)+0x597 [c:\build\chs3\build\mozilla\js\src\jsinterp.c @ 1197]
1e js3250!js_Interpret(struct JSContext * cx = 0x00aef8a0, unsigned char * pc = 0x0a2f019a ":", long * result = 0x0012f974)+0x4fb5 [c:\build\chs3\build\mozilla\js\src\jsinterp.c @ 3524]
1f js3250!js_Invoke(struct JSContext * cx = 0x00000001, unsigned int argc = 4, unsigned int flags = 2)+0x597 [c:\build\chs3\build\mozilla\js\src\jsinterp.c @ 1197]
20 xpc3250!nsXPCWrappedJSClass::CallMethod(class nsXPCWrappedJS * wrapper = 0x0434eda8, unsigned short methodIndex = 3, class nsXPTMethodInfo * info = 0x01eff7e0, struct nsXPTCMiniVariant * nativeParams = 0x0012fb1c)+0x6b1 [c:\build\chs3\build\mozilla\js\src\xpconnect\src\xpcwrappedjsclass.cpp @ 1369]
21 xpc3250!nsXPCWrappedJS::CallMethod(unsigned short methodIndex = 0xeda8, class nsXPTMethodInfo * info = 0x00000003, struct nsXPTCMiniVariant * params = 0x0012fbd8)+0x27 [c:\build\chs3\build\mozilla\js\src\xpconnect\src\xpcwrappedjs.cpp @ 515]
22 xpcom_core!PrepareAndDispatch(class nsXPTCStubBase * self = 0x0a34eda8, unsigned int methodIndex = 3, unsigned int * args = 0x0012fbd8, unsigned int * stackBytesToPop = 0x0012fbc8)+0xee [c:\build\chs3\build\mozilla\xpcom\reflect\xptcall\src\md\win32\xptcstubs.cpp @ 117]
23 xpcom_core!SharedStub(void)+0x16 [c:\build\chs3\build\mozilla\xpcom\reflect\xptcall\src\md\win32\xptcstubs.cpp @ 147]

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Attachment: testcase

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Attachment: possible patch

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Attachment: possible patch

Adding mContextNode to which the changes in document can be compared 
to. Removed NodeWillBeDestroyed, since I can't think (at least usual) 
cases when that would be called but not other mutationobserver 
methods, and when NodeWillBeDestroyed is called, there is no way to 
check whether the node was in anonymous content.
Couldn't use nsContentUtils::IsInSameAnonymousTree here, since
I wanted to cover also the case when change is happening in non-anon-
content. Though, one could argue whether there is any need to handle
cases when contextnode is in anonymous content.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 276888 [details] [diff] [review]
possible patch

Maybe peterv has time to review.
And feel free to mark 
r+sr=+|- ;)

Comment 5

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

Hmm.. it's possibly not good to hold a strong reference to mContextNode. Could you hold a weak one instead? All nsINodes implements nsISupportsWeakReference

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Attachment: with weakptr

Comment 7

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

Comment on attachment 277172 [details] [diff] [review]
with weakptr

>-nsXPathResult::NodeWillBeDestroyed(const nsINode* aNode)
>-{
>-    // Set to null to avoid unregistring unnecessarily
>-    mDocument = nsnull;
>-    Invalidate();
>-}

Why remove this? You still need to null out mDocument if the document is torn down without being mutated. No?

>+nsXPathResult::Invalidate(nsIContent* aChangeRoot)
> {
>+    nsCOMPtr<nsINode> contextNode = do_QueryReferent(mContextNode);
>+    if (contextNode && aChangeRoot) {

If you add |&& aChangeRoot->GetBindingParent()| here you can remove that test in two places further down.

>+        if (contextNode->IsNodeOfType(nsINode::eDOCUMENT)) {
>+            if (aChangeRoot->GetBindingParent() != nsnull) {
>+                return;
>+            }
>+        } else {
>+            nsIContent* changeBindingParent = aChangeRoot->GetBindingParent();
>+            if (changeBindingParent) {
>+                // If context node is in anonymous content, changes to
>+                // non-anonymouscontent need to invalidate the XPathResult. If
>+                // the changes are happening in a different anonymous tree, no
>+                // invalidation should happen.
>+                nsIContent* ctxBindingParent = nsnull;
>+                if (contextNode->IsNodeOfType(nsINode::eATTRIBUTE)) {
>+                    if (contextNode->GetParent()) {
>+                        ctxBindingParent =
>+                            contextNode->GetParent()->GetBindingParent();

GetParent on attributes always returns nsnull. You need to call nsIAttribute::GetContent()

>+                    }
>+                } else {
>+                    ctxBindingParent =
>+                        static_cast<nsIContent*>(contextNode.get())
>+                            ->GetBindingParent();

I wouldn't mind having an |IsNodeOfType(eCONTENT)| check here, just to future proof. Instead of the eDOCUMENT/eATTRIBUTE check you could do

nsIContent* ctxBindingParent = nsnull;
if (eATTRIBUTE) {
  <cast and call GetContent/GetBindingParent>
}
else if (eCONTENT) {
  <cast and call GetBindingParent>
}

if (ctxBindingParent != aChangeRoot->GetBindingParent()) {
  return;
}

r=me with those changes, though it would be great to see some mochitests here, especially with the context node being an attribute.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #7)

> 
> Why remove this? You still need to null out mDocument if the document is torn
> down without being mutated. No?

My mistake. I'll keep the method as it is and pass
aNode to Invalidate as a parameter if it is eContent.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch to check in

I'll write mochitests before commiting.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Attachment: with mochitests

Comment 11

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

There is no point in adding the IsNodeOfType(eCONTENT) in NodeWillBeDestroyed, we're always observing the document so it'll always be false.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Hmm, shouldn't cause any harm though.
But when is NodeWillBeDestroyed actually called. nsXPathResult has owning 
reference to document. The method could be removed probably, after all.
Why was it added originally?
(/me being way too jet-lagged to think anything now.)

Comment 13

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

Hmm.. mDocument should probably be a raw pointer or a weakptr

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Maybe, if someone can write a testcase which leaks.
I tried something simple and didn't see any mem leaks.
Anyway, that is a different bug.

Comment 15

[alexander :surkov (:asurkov)]

Attachment: [mq]: test_Bug 319374

Comment 16

[Phabricator Automation]

Comment on attachment 9107941 [details]
[mq]: test_Bug 319374

Revision D52580 was moved to bug 1594122. Setting attachment 9107941 [details] to obsolete.