Closed Bug 319374 Opened 15 years ago Closed 13 years ago

XPath should ignore changes to anonymous content

Categories

(Core :: XML, defect)

x86
Windows XP
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9alpha8

People

(Reporter: timeless, Assigned: smaug)

Details

Attachments

(2 files, 5 obsolete files)

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]
Assignee: peterv → xml
Assignee: xml → jonas
Flags: blocking1.9? → blocking1.9+
Target Milestone: --- → mozilla1.9beta1
Attached file testcase
Attached patch possible patch (obsolete) — Splinter Review
Attachment #276874 - Flags: review?
Attached patch possible patch (obsolete) — Splinter Review
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.
Assignee: jonas → Olli.Pettay
Status: NEW → ASSIGNED
Attachment #276888 - Flags: review?(jonas)
Attachment #276874 - Attachment is obsolete: true
Attachment #276874 - Flags: review?
Comment on attachment 276888 [details] [diff] [review]
possible patch

Maybe peterv has time to review.
And feel free to mark 
r+sr=+|- ;)
Attachment #276888 - Flags: review?(jonas) → review?(peterv)
Hmm.. it's possibly not good to hold a strong reference to mContextNode. Could you hold a weak one instead? All nsINodes implements nsISupportsWeakReference
Attached patch with weakptr (obsolete) — Splinter Review
Attachment #276888 - Attachment is obsolete: true
Attachment #277172 - Flags: superreview?(peterv)
Attachment #277172 - Flags: review?(peterv)
Attachment #276888 - Flags: review?(peterv)
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.
Attachment #277172 - Flags: superreview?(peterv)
Attachment #277172 - Flags: superreview+
Attachment #277172 - Flags: review?(peterv)
Attachment #277172 - Flags: review+
(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.
Attached patch patch to check in (obsolete) — Splinter Review
I'll write mochitests before commiting.
Attachment #277172 - Attachment is obsolete: true
Attached patch with mochitestsSplinter Review
Attachment #277319 - Attachment is obsolete: true
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
There is no point in adding the IsNodeOfType(eCONTENT) in NodeWillBeDestroyed, we're always observing the document so it'll always be false.
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.)
Hmm.. mDocument should probably be a raw pointer or a weakptr
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.
Attached file [mq]: test_Bug 319374 (obsolete) —

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

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

Attachment #9107941 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.