257868 - The PresShell::SetAnonymousContentFor() API sucks

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Johnny Stenback (:jst)]

To use that method, you have to heap allocate and refcount an nsISupportsArray.
We should have an API that lets you pass in just a single nsIContent pointer
(plus maybe an inline helper to iterate an array in case there's many), or a
nsCOMArray at the very least.

(see bug 257690 for the discussion where this came up).

Comment 1

[Johnny Stenback (:jst)]

Oh, and maybe deCOMtaminate as we go too :-)

Comment 2

[Boris Zbarsky [:bzbarsky]]

In fact, this API is the only reason I haven't changed the functions on
nsIAnonymousContentCreator to nsCOMArray yet.  If we could store an
nsCOMArray<nsIContent> in the presshell and return a ref to it on demand so
people could handle their own appending, that would be ideal, imo...  (otherwise
we have to copy things from one COMArray to another every so often).

Comment 3

[Nickolay_Ponomarev]

Taking.

How about this:

nsIPresShell:
// Replaces previously set anonymous content, if any. Takes ownership 
// of aAnonymousElements
virtual void SetAnonymousContentFor(nsIContent* aContent,
  nsCOMArray<nsIContent>* aAnonymousElements);
// retains ownership, may be null in case no anonymous content is
// registered for aContent
virtual nsCOMArray<nsIContent>* GetAnonymousContentFor(const nsIContent* aContent); 

nsIAnonymousContentCreator:
// it is caller's responsibility to delete the returned object using |delete|.
virtual nsCOMArray<nsIContent>*
CreateAnonymousContent(nsPresContext* aPresContext);
virtual nsIFrame* CreateFrameFor(nsPresContext* aPresContext, nsIContent* aContent)

The nsCOMArray<nsIContent>* from CreateAnonymousContent will commonly be added to presshell's hashmap directly. If it needs to be appended to existing elements, it will be done right in cssframeconstructor.

It doesn't look like any other classes will require major changes.

Comment 4

[Nickolay_Ponomarev]

Attachment: patch, v1

Comment 5

[Nickolay_Ponomarev]

Attachment: patch v1, as -w

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

How about putting the CreateSingleElementArray into nsContentUtils?

Before we do this, how do you feel about looking into this comment here?

+  // What we SHOULD do is get rid of the presshell's need to track anonymous
+  // content. It's only used for cleanup as far as I can tell.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The current anonymous content tracking scheme is used, as far as I can tell, for three things:

1) Debug-only output of anonymous content in nsGenericElement. Not an issue.
http://lxr.mozilla.org/seamonkey/source/content/base/src/nsGenericElement.cpp#3660

2) Some hack in accesibility where buttons keep their name in some anonymous child (in what situation?). We must be able to find a workaround for this.
http://lxr.mozilla.org/seamonkey/source/accessible/src/html/nsHTMLFormControlAccessible.cpp#208

3) PresShell::ReleaseAnonymousContent, which calls UnbindFromTree for the anonymous content.
http://lxr.mozilla.org/seamonkey/source/layout/base/nsPresShell.cpp#4508
This is actually really bad; if some script keeps creating elements with anonymous children, then removing those elements from the tree, as far as I can tell, those children keep accumulating in the presshell until the presshell finally goes away. I call that a leak.

Furthermore there is the confusion noted in comments where an element could get anonymous content from multiple frames and we have no way of tracking which content belongs to which frames, so if some of the frames are destroyed or want to change their content, we're stuffed.

Here's what I suggest: nsCSSFrameConstructor stores the anonymous content created by a frame as a property on the frame. In the Destroy method for each frame class that implements nsIAnonymousContentCreator, call a utility method that retrieves that property and calls UnbindFromTree for the elements in the list. That should be safe, since nsContainerFrame does it:
http://lxr.mozilla.org/seamonkey/source/layout/generic/nsContainerFrame.cpp#253
although you should check with sicking or bz.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Actually, here's what would be slightly cooler. Don't store the content in a frame property, instead, just have the frame classes store nsCOMPtrs to their created content elements, and have their Destroy methods call UnbindFromTree on those elements directly. Make nsIAnonymousContentCreator::CreateAnonymousContent look like

virtual void CreateAnonymousContent(nsTArray<nsIContent*>* aElements);

-- nsPresContext* is not needed, the frame can get it if needed
-- strong pointers don't need to be returned, since the frame object holds strong pointers
-- the caller in nsCSSFrameConstructor can pass in an nsAutoTArray<nsIContent*,4> or something like that so heap allocation of the array won't be needed.

Comment 9

[Nickolay_Ponomarev]

Yeah, I noticed that comment but wasn't sure if moving the owning references to the frame that generates the anonymous content was the right thing to do. OK, I'll make a new patch that does that.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Hmm.  So SetAnonymousContentFor is never used for XBL-bound anonymous content.  nsBindingManager::ChangeDocumentFor sets it to null when it's called, which is whenever the node is removed from the tree.  So we don't exactly leak the way roc was talking about, but that's a nasty hack that I would be happy to remove.

Another interesting place for this stuff is in nsCSSFrameConstructor::ConstructSelectFrame :

4874       // make sure any existing anonymous content is cleared out. Gfx scroll frame construction
4875       // should reset it to just be the anonymous scrollbars, but we don't want to depend
4876       // on that.
4877       mPresShell->SetAnonymousContentFor(aContent, nsnull);

That sounds like something that roc's proposal would also help.

Finally, there's CSS-generated anonymous content (CSS :before/:after).  For that, roc's proposal is definitely the way to go.  In fact, the whole current setup looks pretty broken for :before/:after content (see what nsCSSFrameConstructor::CreateGeneratedFrameFor does, and note that it can be called multiple times for the same element).  The only reason :before/:after works is that the :before/:after frame has the NS_FRAME_GENERATED_CONTENT state bit, so nsContainerFrame::Destroy calls CleanupGeneratedContentIn.  Since all the content nodes in question are owned by the relevant frames, there's no actual need to hold strong refs to them anywhere else.  So it's already pretty much at what roc's proposing.

I guess that's a "yes" vote from me.  ;)

Comment 11

[Nickolay_Ponomarev]

OK, so 
1) from accessibility I can't just cast to the specific class (nsGfxButtonControlFrame), since it's a different module than layout, right? If so, I think I have to either duplicate code from http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/forms/nsGfxButtonControlFrame.cpp&rev=1.137#231 or invent an additional interface just for this purpose. I don't like either variant, any better ideas?

2) Wherever SetAnonymousContentFor(..., nsnull) was called, I just remove it, since the anonymous content is now cleaned up by whatever frame generated it (in the frame's Destroy() method).

3) For CSS-generated content, I rely on the CleanupGeneratedContentIn function to clean up generated content when the frame for the content is destroyed. I don't understand how CleanupGeneratedContentIn works though, in particular why the aRealContent param is needed (it shouldn't correspond to any of child frames of aRoot, since it corresponds to aRoot).

I had to move the CleanupGeneratedCotnentIn call up into nsBlockFrame and nsInlineFrame, so that the generated content is destroyed before the lines in a block frame, otherwise deleting the lines led to deleting the frames to the generated content, which (since they held the only reference to content) led to deleting the content without first removing it from the document.

Sorry for the delay, I wasn't very productive on the weekend :)

Comment 12

[Boris Zbarsky [:bzbarsky]]

You could also just invent a new form property name and change the relevant GetFormProperty impl, then make accessibility use nsIFormControlFrame.

> in particular why the aRealContent param is needed (it shouldn't correspond to
> any of child frames of aRoot, since it corresponds to aRoot).

The CSS:

  div:before { content: "text" url(aaa) counter(foo); }

creates a frame for the :before, which has two text frames and an image frame as kids.  The text frames and image frame point to anonymous content that's created by the frame constructor.  The :before frame has the <div> node as mContent and a style context with a pseudo.

In general, dumping out a frame tree with a :before in it would have helped here, I suspect.  ;)

> I had to move the CleanupGeneratedCotnentIn call up into nsBlockFrame 

Er... given that CSS-generated anon content doesn't really use SetAnonymousContentFor sanely already, was this already a problem before your changes?

Note that things other than blocks/inlines can have generated content, per CSS, so we really do want to try to keep this in nsContainerFrame if we can...

Comment 13

[Nickolay_Ponomarev]

Ah, duh. I saw nsCSSFrameConstructor creating a frame for :before, but didn't notice that it sets the real element as content. So OK, aRoot indeed doesn't correspond to aRealContent.

But your later comments make me think I misunderstand how generated content works. My understanding is that the NS_FRAME_GENERATED_CONTENT frame is always either nsInlineFrame or nsBlockFrame, and it is this frame which calls CleanupGeneratedContentIn:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/base/nsCSSFrameConstructor.cpp&rev=1.1294&mark=2160-2165,2188#2131

In your example CleanupGeneratedContentIn gets called for the :before frame and starts walking the frame tree down from there (removing the three anonymous elements from the tree). I still don't see why the aRealContent check is needed.

For the same reason, I believe moving the CleanupGeneratedContentIn call up was correct.

>Er... given that CSS-generated anon content doesn't really use
>SetAnonymousContentFor sanely already, was this already a problem before your
>changes?
The reason I didn't get the assertion before (I'm talking about the assertion in ~nsGenericElement) was that the presshell's table of native anonymous content held an additional reference to the anon content, so the anon content survived after its frame was destroyed (from nsBlockFrame::Destroy) and was removed from the tree slightly later in CleanupGeneratedContentIn called from nsContainerFrame::Destroy.

Comment 14

[Boris Zbarsky [:bzbarsky]]

> My understanding is that the NS_FRAME_GENERATED_CONTENT frame is always
> either nsInlineFrame or nsBlockFrame

That's true in our code and is per CSS2.  But CSS2.1 changes the behavior here and we'll need to fix it eventually...

> I still don't see why the aRealContent check is needed.

In our current code it probably isn't, you're right.  Once we update to the CSS2.1 stuff it will be.

> was that the presshell's table of native anonymous content held an additional
> reference to the anon content

Oh, SetAnonymousContentFor appends.  OK.  So it did work that way...

I still feel we need a better solution, but I suppose we could just hack it in block/inline frame and then redesign it somehow when we implement generated content per CSS 2.1...

Comment 15

[Nickolay_Ponomarev]

OK, I'll put the Cleanup call in the inline/block frames for now, but I don't see what changes you are talking about (granted I only scanned http://www.w3.org/TR/CSS21/changes.html which is not even normative). The only relevant thing I saw is C.4.21 which clarifies that :before and :after pseudo-elements' display can only be block, inline, or none (with other display values being treated as inline or block, depending on the display of the subject element).

Comment 16

[Boris Zbarsky [:bzbarsky]]

Yeah, the changes list is bogus.  The restrictions on display were removed completely, as was the "User agents must ignore the following properties with :before and :after pseudo-elements: 'position', 'float', list properties, and table properties" restriction.  So you can have a :before table-row in a table-row-group and so forth.  The changes list should be updated accordingly.

Comment 17

[Nickolay_Ponomarev]

(In reply to comment #11)
> I had to move the CleanupGeneratedCotnentIn call up into nsBlockFrame and
> nsInlineFrame, so that the generated content is destroyed before the lines in 
> a block frame, otherwise deleting the lines led to deleting the frames to the
> generated content, which (since they held the only reference to content) 
> led to deleting the content without first removing it from the document.

BTW, it appears that assertion was the reason for the patch in bug 257690.

I'll attach the patch I have now, since it's almost done and I haven't had any progress on it for almost a month.

Will fix the debugging methods along with the review comments. I plan adding a debug-only method to nsIAnonymousContent creator to dump the created content and use it from nsGenericElement::List.

Comment 18

[Nickolay_Ponomarev]

Attachment: current diff -w

Comment 19

[Nickolay_Ponomarev]

Attachment: same, without -w and with -N not forgotten

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Just remove the debugging code, I think. No point in having a special interface just to support it.

While you're changing the signature for CreateFrameFor, remove the aPresContext parameter. (You can always get the prescontext from the frame via GetPresContext()).

Otherwise, looks good to me.

Comment 21

[Nickolay_Ponomarev]

Attachment: patch

Comment 22

[Nickolay_Ponomarev]

Attachment: same as above as -w

Comment 23

[Boris Zbarsky [:bzbarsky]]

I don't think I'll be able to get to this review in a reasonable (next few weeks) timeframe...

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

 #ifdef DEBUG
+
 void

Undo this change.

   if (eStyleContentType_Image == type) {
     if (!data.mContent.mImage) {
       // CSS had something specified that couldn't be converted to an
       // image object
-      *aFrame = nsnull;
       return NS_ERROR_FAILURE;
     }

Why are you removing this?

+  nsAutoTArray<nsIContent*, 4> newAnonymousItems;
+  creator->CreateAnonymousContent(newAnonymousItems);

I think you should check the result here.

-  rv = NS_NewHTMLElement(getter_AddRefs(content), nodeInfo);
-  NS_ENSURE_SUCCESS(rv, rv);
+  NS_NewHTMLElement(getter_AddRefs(mBrowse), nodeInfo);
+  if (!mBrowse)
+    return NS_ERROR_OUT_OF_MEMORY;

Why are you doing this? I think you should retain the NS_ENSURE_SUCCESS instead of checking !mBrowse. You maybe should assert !mBrowse, but it should never be null if NS_NewHTMLElement succeeds.

+  static void CleanupGeneratedContentIn(nsIContent* aRealContent,
+                                         nsIFrame* aRoot);
+

Fix indentation.

It might just be worth creating an nsLayoutUtils method DestroyAnonymousContent(nsCOMPtr<nsIContent>* aContent) which does "if (*aContent) { (*aContent)->UnbindFromTree(); *aContent = nsnull; }".


 #define nsIAnonymousContentCreator_h___
 
 #include "nsISupports.h"
 #include "nsIContent.h"
-#include "nsCOMPtr.h"
+#include "nsCOMArray.h"

Where is nsCOMArray being used?

Comment 25

[Nickolay_Ponomarev]

(In reply to comment #24)
>    if (eStyleContentType_Image == type) {
>      if (!data.mContent.mImage) {
>        // CSS had something specified that couldn't be converted to an
>        // image object
> -      *aFrame = nsnull;
>        return NS_ERROR_FAILURE;
>      }
> 
> Why are you removing this?
> 
*aFrame is already initialized to nsnull at the beginning of the method:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/base/nsCSSFrameConstructor.cpp&rev=1.1309&mark=1911,1926#1903

> -  rv = NS_NewHTMLElement(getter_AddRefs(content), nodeInfo);
> -  NS_ENSURE_SUCCESS(rv, rv);
> +  NS_NewHTMLElement(getter_AddRefs(mBrowse), nodeInfo);
> +  if (!mBrowse)
> +    return NS_ERROR_OUT_OF_MEMORY;
> 
> Why are you doing this? I think you should retain the NS_ENSURE_SUCCESS instead
> of checking !mBrowse. You maybe should assert !mBrowse, but it should never be
> null if NS_NewHTMLElement succeeds.
> 
The code called NS_New* was quite inconsistent, some checking the out parameter,
some checking rv. I thought that since during decom NS_New* will be changed to
return the created object, checking the out param today will lead to fewer 
changes in the future.

> +  static void CleanupGeneratedContentIn(nsIContent* aRealContent,
> +                                         nsIFrame* aRoot);
> +
> 
> Fix indentation.
> 
I hate the fonts in this editor. The indenting looked totally correct.

Comment 26

[Nickolay_Ponomarev]

Attachment: patch v4

With the review comments addressed, except I didn't change those NS_New* null-checks. Tell me if they must be changed too.

Can you r+sr too or do I need a content peer too?

Comment 27

[Nickolay_Ponomarev]

Attachment: patch v4 as -w

Comment 28

[Nickolay_Ponomarev]

(Except I forgot to build in content/svg, where nsLayoutUtils is not defined. I decided to instead make nsSVGUseFrame a friend of nsSVGUseElement and have it destroy the nsSVGUseElement::mClone directly in nsSVGUseFrame::Destroy without an extra function.)

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Put the helper in nsContentUtils instead, then.

I think you should make all the NS_New* calls consistently do NS_ENSURE_SUCCESS(rv, rv).

Comment 30

[Nickolay_Ponomarev]

Attachment: patch v5

With the helper moved to nsContentUtils. Didn't change the NS_New* functions usage, since it seems roc was convinced it was ok to leave it like this until we decom those.

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 255530 [details] [diff] [review]
patch v5

ok. Can you run the reftests before checking in?

Comment 32

[Nickolay_Ponomarev]

Sure, I run them during development anyway. Unfortunately, the set we have now don't test most of the elements I changed, and I don't know how to test the display correctness for most of these. I do have a test for <isindex> (comparing it to <hr><input><hr>) locally, but for the rest of elements I had to test manually.

The accessibility change was tested by using the DOM inspector's accessibility pane (thanks aaronlev for the tip), but I'm too lazy to write a unit test for that.

Comment 33

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It's OK. Just go ahead and check in.

Comment 34

[Nickolay_Ponomarev]

mozilla/accessible/src/base/nsAccessibilityAtomList.h        1.53
mozilla/accessible/src/html/nsHTMLFormControlAccessible.cpp  1.79
mozilla/content/base/public/nsContentUtils.h                 1.119
mozilla/content/base/src/nsContentUtils.cpp                  1.203
mozilla/content/base/src/nsDocument.cpp                      3.708
mozilla/content/base/src/nsGenericElement.cpp                3.536
mozilla/content/base/src/nsGenericElement.h                  3.246
mozilla/content/base/src/nsGkAtomList.h                      3.52
mozilla/content/svg/content/src/nsSVGUseElement.cpp          1.24
mozilla/content/svg/content/src/nsSVGUseElement.h            1.1
mozilla/content/xbl/src/nsBindingManager.cpp                 1.159
mozilla/layout/base/nsCSSFrameConstructor.cpp                1.1314
mozilla/layout/base/nsIPresShell.h                           3.188
mozilla/layout/base/nsPresShell.cpp                          3.969
mozilla/layout/forms/nsComboboxControlFrame.cpp              1.393
mozilla/layout/forms/nsComboboxControlFrame.h                1.158
mozilla/layout/forms/nsFileControlFrame.cpp                  3.204
mozilla/layout/forms/nsFileControlFrame.h                    1.91
mozilla/layout/forms/nsGfxButtonControlFrame.cpp             1.140
mozilla/layout/forms/nsGfxButtonControlFrame.h               1.50
mozilla/layout/forms/nsIsIndexFrame.cpp                      1.86
mozilla/layout/forms/nsIsIndexFrame.h                        1.39
mozilla/layout/forms/nsTextControlFrame.cpp                  3.247
mozilla/layout/forms/nsTextControlFrame.h                    3.95
mozilla/layout/generic/nsBlockFrame.cpp                      3.816
mozilla/layout/generic/nsContainerFrame.cpp                  1.272
mozilla/layout/generic/nsContainerFrame.h                    3.75
mozilla/layout/generic/nsFrame.cpp                           3.706
mozilla/layout/generic/nsGfxScrollFrame.cpp                  3.293
mozilla/layout/generic/nsGfxScrollFrame.h                    3.110
mozilla/layout/generic/nsIAnonymousContentCreator.h          1.17
mozilla/layout/generic/nsInlineFrame.cpp                     3.277
mozilla/layout/generic/nsInlineFrame.h                       1.62
mozilla/layout/svg/base/src/nsSVGUseFrame.cpp                1.14
mozilla/layout/xul/base/src/nsDocElementBoxFrame.cpp         1.26

Comment 35

[Nickolay_Ponomarev]

I don't know how to test this, apart from the isindex reftest I mentioned in comment 32 and manual tests involving looking at how various elements I touched behave. bz, did you have any specific test ideas in mind?

Comment 36

[Boris Zbarsky [:bzbarsky]]

We could try testing that the various callers of this API end up creating frame trees that look the same as just creating non-anonymous content...

But past that I'm not really sure, no.  So we might want to minus this, but we should think about it first.

Comment 37

[[e-mail]]

Comment on attachment 255530 [details] [diff] [review]
patch v5

>-class nsSVGUseElement : public nsSVGUseElementBase,
>-                        public nsIDOMSVGURIReference,
>-                        public nsIDOMSVGUseElement,
>-                        public nsIMutationObserver,
>-                        public nsIAnonymousContentCreator
>-{
>-protected:
>-  friend nsresult NS_NewSVGUseElement(nsIContent **aResult,
>-                                      nsINodeInfo *aNodeInfo);
>-  nsSVGUseElement(nsINodeInfo *aNodeInfo);
>-  virtual ~nsSVGUseElement();
>-  virtual nsresult Init();
>-  
>-public:
>-  NS_DECLARE_STATIC_IID_ACCESSOR(NS_SVG_USE_ELEMENT_IMPL_CID)
>-
>-  // interfaces:
>-  
>-  NS_DECL_ISUPPORTS_INHERITED
>-  NS_DECL_NSIDOMSVGUSEELEMENT
>-  NS_DECL_NSIDOMSVGURIREFERENCE
>-  NS_DECL_NSIMUTATIONOBSERVER
...
>-};
> 
> NS_DEFINE_STATIC_IID_ACCESSOR(nsSVGUseElement, NS_SVG_USE_ELEMENT_IMPL_CID)
Generally we like to keep the DEFINE and DECLARE in the same file (.h/.cpp)

Comment 38

[[e-mail]]

Attachment: Tidy up

Comment 39

[Boris Zbarsky [:bzbarsky]]

For what it's worth, instead of:

+        frame->QueryInterface(NS_GET_IID(nsIFormControlFrame),
+                              (void**) &fcFrame);

it's better to use:

  CallQueryInterface(frame, &fcFrame);

Comment 40

[Boris Zbarsky [:bzbarsky]]

Hmm.  This just broke my build because the patch uses nsTArray in nsIAnonymousContentCreator but neither forward-declares it in that file nor includes it.  I would guess someone somewhere else was including it before nsIAnonymousContentCreator and stopped doing that...

Comment 41

[Boris Zbarsky [:bzbarsky]]

Just adding:

  template <class T> class nsTArray;

to nsIAnonymousContentCreator.h seems to help.

Comment 42

[Boris Zbarsky [:bzbarsky]]

Attachment: Address my two nits