Closed Bug 672504 Opened 8 years ago Closed 8 years ago

Don't keep pointer to weak presshell in accessible

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla13

People

(Reporter: surkov, Assigned: hub)

References

(Blocks 1 open bug)

Details

(Keywords: access, Whiteboard: [mentor=trev.saunders@gmail.com][lang=c++])

Attachments

(2 files, 2 obsolete files)

It makes sense to keep mWeakShell in document accessible since all accessibles belonging to same document belongs to same presshell. It saves 4 bytes per instance.
(In reply to comment #0)
> It makes sense to keep mWeakShell in document accessible since all
> accessibles belonging to same document belongs to same presshell. It saves 4
> bytes per instance.

taking into account, DOM document can have more than one presshell then we can't just remove mWeakShell. However it make sense to keep a pointer document accessible rather than a weak pointer to presshell. No memory saving, just cleaning up.
Blocks: 673405
Assignee: nobody → hub
make mWeakShell member of nsAccessNode, a  nsDocAccessible* and rename it to mDoc
2. change constructor of nsAccessNode and everything that inherits from it to take an nsDocAccessible* instead of a a nsWeakShell and rename the arg to aDoc
3. add inline method to nsAccessibilityService to get nsDocAccessible* for a nsIPresShell* use nsIPresShell::GetDocument() to get a nsIDocument* then nsAccDocManager to get the nsDocAccessible for it.
4. add pres shell member to nsDocAccessible, and make its constructor take it as an argument.
5. provide an accessor for pres shell member of nsDocAccessible
6. replace uses of mWeakShell in accessibles either by using the document acessible directly, or getting the pres shell from the doc accessible as appropriate.
Assignee: hub → nobody
Whiteboard: [mentor=trev.saunders@gmail.com][lang=c++]
Taking this as I'd like to get bug 673405 in as well. I have it under control.
Assignee: nobody → hub
I pondering folding the patch for 673405 into that one. What do you guys think?
This is the first version of that very long patch. It should build on all three platforms. I have tested one Mac. (Will test on Linux soon). Try server build didn't turn red.

Still wondering if it wouldn't be smarted to fold bug 673405 into this one.
Attachment #590956 - Flags: review?(surkov.alexander)
> 
> Still wondering if it wouldn't be smarted to fold bug 673405 into this one.

I probably wouldn't mind terribly either way (though not surkov), but I tend to think this patch is already huge.
Comment on attachment 590956 [details] [diff] [review]
Don't keep pointer to weak presshell in accessible. r=

> 
> NS_IMETHODIMP nsAccessibleWrap::GetNativeInterface(void **aOutAccessible)
> {
>     *aOutAccessible = nsnull;
> 
>     if (!mAtkObject) {
>-        if (!mWeakShell || !nsAccUtils::IsEmbeddedObject(this)) {
>+        nsDocAccessible* doc = GetDocAccessible();
>+        if (!doc || !doc->GetWeakShell() || !nsAccUtils::IsEmbeddedObject(this)) {
>             // We don't create ATK objects for node which has been shutdown, or
>             // nsIAccessible plain text leaves
>             return NS_ERROR_FAILURE;
>         }

use IsDefunct()

nit, put comment before if and get rid of braces while your here.

> 
>+  static nsDocAccessible *GetDocAccessibleFor(const nsCOMPtr<nsIPresShell>& presShell)

taking a nsCOMPtr as an arg is really weird  I'd be suprised if ust a nsIPresShell won't work here.

nit, type* foo and aPresShell

>-void nsAccessNode::LastRelease()
>-{
>-  // First cleanup if needed...
>-  if (mWeakShell) {
>-    Shutdown();

I'm not sure this method is absolutely necessary, but I'd prefer in this patch you just change mWeakShell here to be IsDefunct()

> nsAccessNode::Shutdown()
> {
>   mContent = nsnull;
>-  mWeakShell = nsnull;

make mDoc null here

> }
> 
> nsApplicationAccessible*
> nsAccessNode::GetApplicationAccessible()
> {
>   NS_ASSERTION(!nsAccessibilityService::IsShutdown(),
>                "Accessibility wasn't initialized!");
> 
>@@ -222,43 +210,29 @@ void nsAccessNode::ShutdownXPAccessibili
>   if (gApplicationAccessible) {
>     gApplicationAccessible->Shutdown();
>     NS_RELEASE(gApplicationAccessible);
>   }
> 
>   NotifyA11yInitOrShutdown(false);
> }
> 
>-already_AddRefed<nsIPresShell>
>-nsAccessNode::GetPresShell()
>-{
>-  nsIPresShell* presShell = nsnull;
>-  if (mWeakShell)
>-    CallQueryReferent(mWeakShell.get(), &presShell);
>-
>-  return presShell;
>-}
>-
> // nsAccessNode protected
> nsPresContext* nsAccessNode::GetPresContext()
> {
>-  nsCOMPtr<nsIPresShell> presShell(GetPresShell());
>+  nsDocAccessible* doc = GetDocAccessible();
>+  if (!doc)
>+    return nsnull;
>+  nsCOMPtr<nsIPresShell> presShell(doc->GetPresShell());

nit, blank line

>   if (!presShell) {
>     return nsnull;
>   }
>   return presShell->GetPresContext();

nit, while your here return presShell ? nsnull : presShell->GetPresContext();

> nsAccessNode::GetCurrentFocus()
> {
>   // XXX: consider to use nsFocusManager directly, it allows us to avoid
>   // unnecessary query interface calls.
>-  nsCOMPtr<nsIPresShell> shell = GetPresShell();
>-  NS_ENSURE_TRUE(shell, nsnull);
>-  nsIDocument *doc = shell->GetDocument();
>+  nsDocAccessible* docAcc = GetDocAccessible();
>+  NS_ENSURE_TRUE(docAcc, nsnull);

at first glance I'd think this method is only called in contexts where an accessible is in a doc, but that should be checked.

>+  nsDocAccessible* GetDocAccessible() const
>+    { return mDoc; }

nit, all on one line

> 
>   /**
>    * Return the root document accessible for this accessnode.
>    */
>   nsRootAccessible* RootAccessible() const;
> 
>   /**
>    * Return focused node within accessible window.
>@@ -167,26 +168,16 @@ public:
>     return node && node->IsElement();
>   }
>   bool IsDocumentNode() const
>   {
>     return GetNode() && GetNode()->IsNodeOfType(nsINode::eDOCUMENT);
>   }
> 
>   /**
>-   * Return the corresponding press shell for this accessible.
>-   */
>-  already_AddRefed<nsIPresShell> GetPresShell();
>-
>-  /**
>-   * Return presentation shell for the accessible.
>-   */
>-  nsIWeakReference* GetWeakShell() const { return mWeakShell; }
>-
>-  /**
>    * Return the unique identifier of the accessible.
>    */
>   void* UniqueID() { return static_cast<void*>(this); }
> 
>   /**
>    * Return true if the accessible is primary accessible for the given DOM node.
>    *
>    * Accessible hierarchy may be complex for single DOM node, in this case
>@@ -199,20 +190,18 @@ public:
>    * Return the string bundle
>    */
>   static nsIStringBundle* GetStringBundle()
>     { return gStringBundle; }
> 
> protected:
>     nsPresContext* GetPresContext();
> 
>-    void LastRelease();
>-
>   nsCOMPtr<nsIContent> mContent;
>-  nsCOMPtr<nsIWeakReference> mWeakShell;
>+  nsDocAccessible* mDoc;
> 
>     /**
>      * Notify global nsIObserver's that a11y is getting init'd or shutdown
>      */
>     static void NotifyA11yInitOrShutdown(bool aIsInit);
> 
>     // Static data, we do our own refcounting for our static data
>     static nsIStringBundle *gStringBundle;
>diff --git a/accessible/src/base/nsAccessibilityService.cpp b/accessible/src/base/nsAccessibilityService.cpp
>--- a/accessible/src/base/nsAccessibilityService.cpp
>+++ b/accessible/src/base/nsAccessibilityService.cpp
>@@ -199,103 +199,103 @@ nsAccessibilityService::GetRootDocumentA
>       }
> 
>       return aCanCreate ?
>         GetDocAccessible(documentNode) : GetDocAccessibleFromCache(documentNode);
>     }
>   }
>   return nsnull;
> }
>-
>+ 
> already_AddRefed<nsAccessible>
> nsAccessibilityService::CreateOuterDocAccessible(nsIContent* aContent,
>                                                  nsIPresShell* aPresShell)
> {
>   nsCOMPtr<nsIWeakReference> weakShell(do_GetWeakReference(aPresShell));
>-  nsAccessible* accessible = new nsOuterDocAccessible(aContent, weakShell);
>+  nsAccessible* accessible = new nsOuterDocAccessible(aContent, nsAccUtils::GetDocAccessibleFor(weakShell));

it'd be faster to do GetDocAccessible(aPresShell->GetDocument());
on the other hand it might make more sense to just use the node, and ignore the preshell all together, I'm not sure off hand if a document would ever have more than one presShell, or a node have more than one for that matter.
note, I stopped looking in nsAccessibilityService.cpp
Attachment #590956 - Attachment is obsolete: true
Attachment #590956 - Flags: review?(surkov.alexander)
Attachment #591646 - Flags: review?(surkov.alexander)
(In reply to Trevor Saunders (:tbsaunde) from comment #8)

> >   nsCOMPtr<nsIWeakReference> weakShell(do_GetWeakReference(aPresShell));
> >-  nsAccessible* accessible = new nsOuterDocAccessible(aContent, weakShell);
> >+  nsAccessible* accessible = new nsOuterDocAccessible(aContent, nsAccUtils::GetDocAccessibleFor(weakShell));
> 
> it'd be faster to do GetDocAccessible(aPresShell->GetDocument());
> on the other hand it might make more sense to just use the node, and ignore
> the preshell all together, I'm not sure off hand if a document would ever
> have more than one presShell, or a node have more than one for that matter.

DOM document can have more than one presShell. We don't handle that in accessibility properly however (for example, doc manager keeps the document accessibles cache by DOM documents while it should use presshell for that) but we shouldn't to get rid all existing infrastructure of mutlitple presshells per DOM document support.
Comment on attachment 591646 [details] [diff] [review]
Don't keep pointer to weak presshell in accessible. r=

Review of attachment 591646 [details] [diff] [review]:
-----------------------------------------------------------------

thanks for doing this!

::: accessible/src/atk/nsAccessNodeWrap.cpp
@@ +55,5 @@
>  //-----------------------------------------------------
>  
>  nsAccessNodeWrap::
> +    nsAccessNodeWrap(nsIContent* aContent, nsDocAccessible* aDoc) :
> +    nsAccessNode(aContent, aDoc)

nit: two spaces indentation please

::: accessible/src/atk/nsAccessNodeWrap.h
@@ +48,5 @@
>  
>  class nsAccessNodeWrap :  public nsAccessNode
>  {
>  public: // construction, destruction
> +    nsAccessNodeWrap(nsIContent* aContent, nsDocAccessible* aDoc);

nit: two spaces indentation

::: accessible/src/atk/nsAccessibleWrap.cpp
@@ +281,5 @@
>  #endif
>  
>  nsAccessibleWrap::
> +    nsAccessibleWrap(nsIContent* aContent, nsDocAccessible* aDoc) :
> +    nsAccessible(aContent, aDoc), mAtkObject(nsnull)

nit: two spaces indentation

@@ +362,5 @@
>      *aOutAccessible = nsnull;
>  
>      if (!mAtkObject) {
> +        nsDocAccessible* doc = GetDocAccessible();
> +        if (IsDefunct() || !doc->GetWeakShell() || !nsAccUtils::IsEmbeddedObject(this)) {

IsDefunct() must be enough (!doc->GetWeakShell() is not needed)

::: accessible/src/atk/nsAccessibleWrap.h
@@ +83,5 @@
>   */
>  class nsAccessibleWrap: public nsAccessible
>  {
>  public:
> +    nsAccessibleWrap(nsIContent* aContent, nsDocAccessible* aDoc);

nit: two spaces indentation

::: accessible/src/base/NotificationController.cpp
@@ +669,5 @@
>  void
>  NotificationController::CreateTextChangeEventFor(AccMutationEvent* aEvent)
>  {
>    nsAccessible* container =
> +    GetAccService()->GetContainerAccessible(aEvent->mNode);

don't loose presshell info, get a document and then get accessible

::: accessible/src/base/nsAccUtils.cpp
@@ +524,5 @@
>  nsIntPoint
>  nsAccUtils::GetScreenCoordsForParent(nsAccessNode *aAccessNode)
>  {
>    nsAccessible *parent =
> +    GetAccService()->GetContainerAccessible(aAccessNode->GetNode());

get an accessible from document accessible the accessnode belongs to

::: accessible/src/base/nsAccUtils.h
@@ +153,5 @@
>     * Return atomic value of ARIA attribute of boolean or NMTOKEN type.
>     */
>    static nsIAtom* GetARIAToken(mozilla::dom::Element* aElement, nsIAtom* aAttr);
>  
> +  static nsDocAccessible* GetDocAccessibleFor(const nsIPresShell* aPresShell)

comment it?

::: accessible/src/base/nsAccessNode.cpp
@@ +94,5 @@
>    NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIAccessNode)
>  NS_INTERFACE_MAP_END
>   
>  NS_IMPL_CYCLE_COLLECTING_ADDREF(nsAccessNode)
> +NS_IMPL_CYCLE_COLLECTING_RELEASE(nsAccessNode)

what is a reason to remove LastRelease stuff?

@@ +102,5 @@
>  
>  nsAccessNode::
> +  nsAccessNode(nsIContent* aContent, nsDocAccessible* aDoc) :
> +  mContent(aContent),
> +  mDoc(aDoc)

keep them on the same line?

@@ +219,4 @@
>  // nsAccessNode protected
>  nsPresContext* nsAccessNode::GetPresContext()
>  {
> +  nsDocAccessible* doc = GetDocAccessible();

mDoc doesn't work here?

@@ +225,4 @@
>  
> +  nsCOMPtr<nsIPresShell> presShell(doc->GetPresShell());
> +
> +  return presShell ? presShell->GetPresContext() : nsnull;

please add XXX section here to say this doesn't work for multipresshell per document stuffs

@@ +311,5 @@
>  {
>    if (IsDefunct())
>      return NS_ERROR_FAILURE;
>  
> +  nsDocAccessible* doc = GetDocAccessible();

use mDoc?

@@ +314,5 @@
>  
> +  nsDocAccessible* doc = GetDocAccessible();
> +  NS_ENSURE_TRUE(doc, NS_ERROR_FAILURE);
> +
> +  nsCOMPtr<nsIPresShell> shell(doc->GetPresShell());

do you need to keep it addrefed?

@@ +320,5 @@
>  
>    nsIFrame *frame = GetFrame();
>    NS_ENSURE_TRUE(frame, NS_ERROR_FAILURE);
>  
>    nsCOMPtr<nsIContent> content = frame->GetContent();

same while you're here

@@ +398,1 @@
>    NS_ENSURE_TRUE(doc, nsnull);

just GetDocumentNode()

::: accessible/src/base/nsAccessNode.h
@@ +99,5 @@
>  
>    /**
>     * Return the document accessible for this access node.
>     */
> +  nsDocAccessible* GetDocAccessible() const { return mDoc; }

I prefer GetDocAccessible() -> Document()

@@ +197,2 @@
>    nsCOMPtr<nsIContent> mContent;
> +  nsDocAccessible* mDoc;

that's pretty cool you think we don't need to keep strong reference to document but I'm sort of ready to bet we will deal here with freaking crashes. Anyway we can go with it and then if we see lot of crashes then get back to strong ref.

::: accessible/src/base/nsAccessibilityService.cpp
@@ +209,5 @@
>  nsAccessibilityService::CreateOuterDocAccessible(nsIContent* aContent,
>                                                   nsIPresShell* aPresShell)
>  {
>    nsCOMPtr<nsIWeakReference> weakShell(do_GetWeakReference(aPresShell));
> +  nsAccessible* accessible = new nsOuterDocAccessible(aContent, nsAccUtils::GetDocAccessibleFor(weakShell));

It makes sense to add GetDocAccessible(nsIPresShell*) method to nsAccDocManager and use it here. Later we can fix nsAccDocManager to make this method valuable.

Actually I would love to see previous comments addressed before doing review. Otherwise I do same concerns as Trevor did.

@@ +888,5 @@
>    return document ? document->GetAccessible(aNode) : nsnull;
>  }
>  
>  nsAccessible*
> +nsAccessibilityService::GetAccessibleOrContainer(nsINode* aNode)

I'd say to keep fake nsIPresShell* argument so later we can fix this

@@ -1038,5 @@
>  
>        return nsnull;
>      }
>  
> -    newAcc = new nsHyperTextAccessibleWrap(content, aWeakShell);

I wonder if you still need to have aWeakShell argument

@@ +1610,5 @@
>    }
>  
>    if (tag == nsGkAtoms::option) {
>      nsAccessible* accessible = new nsHTMLSelectOptionAccessible(aContent,
> +                                                                aDoc);

nit: you can keep it on the same line, right?

@@ +1617,5 @@
>    }
>  
>    if (tag == nsGkAtoms::optgroup) {
>      nsAccessible* accessible = new nsHTMLSelectOptGroupAccessible(aContent,
> +                                                                  aDoc);

same

@@ +1636,5 @@
>      nsRoleMapEntry *roleMapEntry = nsAccUtils::GetRoleMapEntry(aContent);
>      if (roleMapEntry && roleMapEntry->role != roles::NOTHING &&
>          roleMapEntry->role != roles::LINK) {
>        nsAccessible* accessible = new nsHyperTextAccessibleWrap(aContent,
> +                                                               aDoc);

same

@@ +1680,1 @@
>                                                          roles::ROW);

same?

@@ +1683,5 @@
>    }
>  
>    if (nsCoreUtils::IsHTMLTableHeader(aContent)) {
>      nsAccessible* accessible = new nsHTMLTableHeaderCellAccessibleWrap(aContent,
> +                                                                       aDoc);

same

@@ +1696,5 @@
>    }
>  
>    if (tag == nsGkAtoms::progress) {
>      nsAccessible* accessible =
> +      new HTMLProgressMeterAccessible(aContent, aDoc);

same

@@ +1794,5 @@
>  #ifdef MOZ_XUL
>        if (parentContent->NodeInfo()->Equals(nsGkAtoms::tabpanels,
>                                              kNameSpaceID_XUL)) {
>          nsAccessible* accessible = new nsXULTabpanelAccessible(aContent,
> +                                                               aDoc);

same

::: accessible/src/base/nsAccessible.cpp
@@ +779,5 @@
>  
>    // Get accessible for the node with the point or the first accessible in
>    // the DOM parent chain.
>    nsAccessible* accessible =
> +    GetAccService()->GetAccessibleOrContainer(content);

can you use document accessible to get an accessible for the content?

@@ +966,5 @@
>  
>    if (IsDefunct())
>      return NS_ERROR_FAILURE;
>  
> +  nsDocAccessible* doc = GetDocAccessible();

mDoc?

@@ +967,5 @@
>    if (IsDefunct())
>      return NS_ERROR_FAILURE;
>  
> +  nsDocAccessible* doc = GetDocAccessible();
> +  nsCOMPtr<nsIPresShell> presShell = doc->GetPresShell();

do you need to keep strong ref to presShell? it doesn't sound like it can go away

@@ +1647,5 @@
>  
>    // Check if it's a simple xlink.
>    if (nsCoreUtils::IsXLink(mContent)) {
> +    nsDocAccessible* doc = GetDocAccessible();
> +    nsCOMPtr<nsIPresShell> presShell = doc->GetPresShell();

same

@@ +2174,5 @@
>      return;
>  
> +  nsDocAccessible* doc = GetDocAccessible();
> +  if (!doc)
> +    return;

you must be guaranteed that mDoc is not null

@@ +2176,5 @@
> +  nsDocAccessible* doc = GetDocAccessible();
> +  if (!doc)
> +    return;
> +
> +  nsCOMPtr<nsIPresShell> presShell = doc->GetPresShell();

don't keep it addrefed?

@@ +2958,5 @@
>  nsAccessible::CacheChildren()
>  {
> +  nsDocAccessible* doc = GetDocAccessible();
> +  if(!doc)
> +    return;

same
nit: new line pelase

@@ +3041,2 @@
>    nsAccessible* accessible =
> +    GetAccService()->GetAccessibleInWeakShell(aStartNode, weakShell);

it doesn't sound like you need weakshell method

::: accessible/src/base/nsDocAccessible.h
@@ +137,5 @@
> +  /**
> +   * Return presentation shell for the accessible.
> +   */
> +  nsIWeakReference* GetWeakShell() const
> +    { return mWeakShell; }

keep mPresShell instead of mWeakShell
and then do

nsIPresShell* PresShell() const { return mPresShell; }

@@ +171,5 @@
>    /**
>     * Return true if the document has given document state.
>     */
>    bool HasLoadState(LoadState aState) const
> +    { return (mLoadState & (PRUint32)aState) == (PRUint32)aState; }

use c++ casting style please

::: accessible/src/base/nsRootAccessible.cpp
@@ +376,5 @@
>      return;
>    }
>  
>    nsAccessible* accessible =
> +    GetAccService()->GetAccessibleOrContainer(origTargetNode);

it's likely we don't receive DOM events for print preview so that should be ok

@@ +562,5 @@
>  void
>  nsRootAccessible::Shutdown()
>  {
>    // Called manually or by nsAccessNode::LastRelease()
> +  if (!mDoc)

you still can use mWeakShell to see if we shutdown

::: accessible/src/html/nsHTMLLinkAccessible.cpp
@@ +118,5 @@
>      return NS_OK;
>    
> +  if (!mDoc)
> +    return NS_ERROR_FAILURE;
> +

it shouldn't be null

::: accessible/src/html/nsHTMLSelectAccessible.cpp
@@ +179,5 @@
>  void
>  nsHTMLSelectListAccessible::CacheOptSiblings(nsIContent *aParentContent)
>  {
> +  if (!mDoc)
> +    return;

shouldn't be null

@@ +181,5 @@
>  {
> +  if (!mDoc)
> +    return;
> +
> +  nsCOMPtr<nsIPresShell> presShell(mDoc->GetPresShell());

don't keep it addrefed

::: accessible/src/html/nsHTMLTableAccessible.cpp
@@ +347,5 @@
>      else if (aRowOrColumnHeaderCell == nsAccUtils::eColumnHeaderCells)
>        desiredRole = roles::COLUMNHEADER;
>  
> +    if (!mDoc)
> +      return NS_ERROR_FAILURE;

shouldn't be null

@@ +463,5 @@
>    // caption only, because nsAccessibilityService ensures we don't create
>    // accessibles for the other captions, since only the first is actually
>    // visible.
> +  if(!mDoc)
> +    return;

shouldn't be null

@@ +711,5 @@
>      rowSpan, colSpan, actualRowSpan, actualColSpan;
>    bool isSelected = false;
>  
> +  if (!mDoc)
> +    return NS_ERROR_FAILURE;

same

@@ +902,5 @@
>    nsresult rv = GetCellAt(aRow, aColumn, *getter_AddRefs(cellElement));
>    NS_ENSURE_SUCCESS(rv, rv);
>  
>    nsCOMPtr<nsIContent> cellContent(do_QueryInterface(cellElement));
> +  nsAccessible *cell = mDoc ?

nit: nsAccessible* cell
mDoc shouldn't be null

@@ +1205,5 @@
>      rv = GetRowCount(&count);
>  
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  NS_ENSURE_TRUE(mDoc, NS_ERROR_FAILURE);

same

@@ +1238,5 @@
>  {
>    nsITableLayout *tableLayout = GetTableLayout();
>    NS_ENSURE_STATE(tableLayout);
>  
> +  NS_ENSURE_TRUE(mDoc, NS_ERROR_FAILURE);

same

::: accessible/src/html/nsHTMLTableAccessible.h
@@ +211,5 @@
>  class nsHTMLCaptionAccessible : public nsHyperTextAccessibleWrap
>  {
>  public:
> +  nsHTMLCaptionAccessible(nsIContent* aContent, nsDocAccessible* aDoc) :
> +    nsHyperTextAccessibleWrap(aContent, aDoc) { }

add virtual destructor while you're here?

::: accessible/src/html/nsHyperTextAccessible.cpp
@@ +1551,5 @@
>    if (!editingSession)
>      return NS_OK; // No editing session interface
>  
> +  NS_ENSURE_TRUE(mDoc, NS_ERROR_FAILURE);
> +  nsCOMPtr<nsIDocument> doc = mDoc->GetDocumentNode();

don't addref

::: accessible/src/html/nsHyperTextAccessible.h
@@ +75,5 @@
>                                public nsIAccessibleHyperText,
>                                public nsIAccessibleEditableText
>  {
>  public:
> +  nsHyperTextAccessible(nsIContent* aContent, nsDocAccessible* aDoc);

empty line please, virtual destructor

::: accessible/src/mac/nsAccessNodeWrap.h
@@ +47,5 @@
>  
>  class nsAccessNodeWrap :  public nsAccessNode
>  {
>    public: // construction, destruction
> +    nsAccessNodeWrap(nsIContent* aContent, nsDocAccessible* aShell);

please indent properly public and constructor
aShell -> aDoc

::: accessible/src/msaa/nsAccessNodeWrap.cpp
@@ +77,5 @@
>  // nsAccessNodeWrap
>  ////////////////////////////////////////////////////////////////////////////////
>  
>  nsAccessNodeWrap::
> +  nsAccessNodeWrap(nsIContent *aContent, nsDocAccessible* aDoc) :

nsIContent *aContent -> nsIContent* aContent

@@ +421,5 @@
>    nsAccessNodeWrap *newNode = NULL;
>  
>    ISimpleDOMNode *iNode = NULL;
> +  nsAccessible *acc = mDoc ?
> +    GetAccService()->GetAccessibleInWeakShell(aNode, mDoc->GetWeakShell()) : nsnull;

nsAccessible *acc -> nsAccessible* acc

::: accessible/src/msaa/nsAccessibleWrap.cpp
@@ +1549,5 @@
>    if (!winEvent)
>      return NS_OK;
>  
>    // Means we're not active.
> +  NS_ENSURE_TRUE(mDoc, NS_ERROR_FAILURE);

IsDefunct check should be right here

::: accessible/src/xforms/nsXFormsAccessible.cpp
@@ +77,5 @@
>  // nsXFormsAccessible
>  ////////////////////////////////////////////////////////////////////////////////
>  
>  nsXFormsAccessible::
> +nsXFormsAccessible(nsIContent* aContent, nsDocAccessible* aDoc) :

two spaces indent

@@ +125,1 @@
>      return;

you don't need mDoc check
Attachment #591646 - Flags: review?(surkov.alexander) → review-
> 
> @@ +197,2 @@
> >    nsCOMPtr<nsIContent> mContent;
> > +  nsDocAccessible* mDoc;
> 
> that's pretty cool you think we don't need to keep strong reference to
> document but I'm sort of ready to bet we will deal here with freaking
> crashes. Anyway we can go with it and then if we see lot of crashes then get
> back to strong ref.

Yeah, it would be cool, and I'm concerned too.  Fortunately taking this before the mrege would be mad anyway.  So lets try to land it as soon as we can after that and hope we can actually fix the crashes not just make the pointer strong again.
Comment on attachment 591646 [details] [diff] [review]
Don't keep pointer to weak presshell in accessible. r=

Review of attachment 591646 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/atk/nsAccessNodeWrap.cpp
@@ +55,5 @@
>  //-----------------------------------------------------
>  
>  nsAccessNodeWrap::
> +    nsAccessNodeWrap(nsIContent* aContent, nsDocAccessible* aDoc) :
> +    nsAccessNode(aContent, aDoc)

done

::: accessible/src/atk/nsAccessNodeWrap.h
@@ +48,5 @@
>  
>  class nsAccessNodeWrap :  public nsAccessNode
>  {
>  public: // construction, destruction
> +    nsAccessNodeWrap(nsIContent* aContent, nsDocAccessible* aDoc);

done

::: accessible/src/atk/nsAccessibleWrap.cpp
@@ +281,5 @@
>  #endif
>  
>  nsAccessibleWrap::
> +    nsAccessibleWrap(nsIContent* aContent, nsDocAccessible* aDoc) :
> +    nsAccessible(aContent, aDoc), mAtkObject(nsnull)

done

@@ +362,5 @@
>      *aOutAccessible = nsnull;
>  
>      if (!mAtkObject) {
> +        nsDocAccessible* doc = GetDocAccessible();
> +        if (IsDefunct() || !doc->GetWeakShell() || !nsAccUtils::IsEmbeddedObject(this)) {

done

::: accessible/src/atk/nsAccessibleWrap.h
@@ +83,5 @@
>   */
>  class nsAccessibleWrap: public nsAccessible
>  {
>  public:
> +    nsAccessibleWrap(nsIContent* aContent, nsDocAccessible* aDoc);

The only reason it is not tow spaces is to keep the indentation consistent (it is at 4 in there) and not re-indent the whole class declaration.

::: accessible/src/base/NotificationController.cpp
@@ +669,5 @@
>  void
>  NotificationController::CreateTextChangeEventFor(AccMutationEvent* aEvent)
>  {
>    nsAccessible* container =
> +    GetAccService()->GetContainerAccessible(aEvent->mNode);

GetContainerAccessible does not use the weakshell it receive so I removed it. Maybe I was over enthusiastic.

::: accessible/src/base/nsAccUtils.cpp
@@ +524,5 @@
>  nsIntPoint
>  nsAccUtils::GetScreenCoordsForParent(nsAccessNode *aAccessNode)
>  {
>    nsAccessible *parent =
> +    GetAccService()->GetContainerAccessible(aAccessNode->GetNode());

changed back to what it was before, but we pass the presShell, not the weak shell.

::: accessible/src/base/nsAccUtils.h
@@ +153,5 @@
>     * Return atomic value of ARIA attribute of boolean or NMTOKEN type.
>     */
>    static nsIAtom* GetARIAToken(mozilla::dom::Element* aElement, nsIAtom* aAttr);
>  
> +  static nsDocAccessible* GetDocAccessibleFor(const nsIPresShell* aPresShell)

done

::: accessible/src/base/nsAccessNode.cpp
@@ +94,5 @@
>    NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIAccessNode)
>  NS_INTERFACE_MAP_END
>   
>  NS_IMPL_CYCLE_COLLECTING_ADDREF(nsAccessNode)
> +NS_IMPL_CYCLE_COLLECTING_RELEASE(nsAccessNode)

It was used to release the presShell. Since it is not longer in there, it is no longer needed and I let the standard implementation instead.

@@ +102,5 @@
>  
>  nsAccessNode::
> +  nsAccessNode(nsIContent* aContent, nsDocAccessible* aDoc) :
> +  mContent(aContent),
> +  mDoc(aDoc)

done

@@ +219,4 @@
>  // nsAccessNode protected
>  nsPresContext* nsAccessNode::GetPresContext()
>  {
> +  nsDocAccessible* doc = GetDocAccessible();

done

@@ +225,4 @@
>  
> +  nsCOMPtr<nsIPresShell> presShell(doc->GetPresShell());
> +
> +  return presShell ? presShell->GetPresContext() : nsnull;

done

@@ +311,5 @@
>  {
>    if (IsDefunct())
>      return NS_ERROR_FAILURE;
>  
> +  nsDocAccessible* doc = GetDocAccessible();

done

@@ +314,5 @@
>  
> +  nsDocAccessible* doc = GetDocAccessible();
> +  NS_ENSURE_TRUE(doc, NS_ERROR_FAILURE);
> +
> +  nsCOMPtr<nsIPresShell> shell(doc->GetPresShell());

added a dont_AddRef()

@@ +320,5 @@
>  
>    nsIFrame *frame = GetFrame();
>    NS_ENSURE_TRUE(frame, NS_ERROR_FAILURE);
>  
>    nsCOMPtr<nsIContent> content = frame->GetContent();

done

@@ +398,1 @@
>    NS_ENSURE_TRUE(doc, nsnull);

done

::: accessible/src/base/nsAccessNode.h
@@ +99,5 @@
>  
>    /**
>     * Return the document accessible for this access node.
>     */
> +  nsDocAccessible* GetDocAccessible() const { return mDoc; }

see bug 673405 and how I was proposing to fold it into this one. Because it does that change too.

@@ +197,2 @@
>    nsCOMPtr<nsIContent> mContent;
> +  nsDocAccessible* mDoc;

Let's try without (as Trevor suggested)

::: accessible/src/base/nsAccessibilityService.cpp
@@ +209,5 @@
>  nsAccessibilityService::CreateOuterDocAccessible(nsIContent* aContent,
>                                                   nsIPresShell* aPresShell)
>  {
>    nsCOMPtr<nsIWeakReference> weakShell(do_GetWeakReference(aPresShell));
> +  nsAccessible* accessible = new nsOuterDocAccessible(aContent, nsAccUtils::GetDocAccessibleFor(weakShell));

I still don't get what the issue here? We want to not have to create a weakShell and just use the presShell?

@@ +888,5 @@
>    return document ? document->GetAccessible(aNode) : nsnull;
>  }
>  
>  nsAccessible*
> +nsAccessibilityService::GetAccessibleOrContainer(nsINode* aNode)

done

@@ -1038,5 @@
>  
>        return nsnull;
>      }
>  
> -    newAcc = new nsHyperTextAccessibleWrap(content, aWeakShell);

The patch already change it to just pass the nsDocAccessible*

@@ +1610,5 @@
>    }
>  
>    if (tag == nsGkAtoms::option) {
>      nsAccessible* accessible = new nsHTMLSelectOptionAccessible(aContent,
> +                                                                aDoc);

done

@@ +1617,5 @@
>    }
>  
>    if (tag == nsGkAtoms::optgroup) {
>      nsAccessible* accessible = new nsHTMLSelectOptGroupAccessible(aContent,
> +                                                                  aDoc);

Here it is over 80 column (Trevor asked me to wrap at 80)

@@ +1636,5 @@
>      nsRoleMapEntry *roleMapEntry = nsAccUtils::GetRoleMapEntry(aContent);
>      if (roleMapEntry && roleMapEntry->role != roles::NOTHING &&
>          roleMapEntry->role != roles::LINK) {
>        nsAccessible* accessible = new nsHyperTextAccessibleWrap(aContent,
> +                                                               aDoc);

done

@@ +1680,1 @@
>                                                          roles::ROW);

Over 80 columns

@@ +1683,5 @@
>    }
>  
>    if (nsCoreUtils::IsHTMLTableHeader(aContent)) {
>      nsAccessible* accessible = new nsHTMLTableHeaderCellAccessibleWrap(aContent,
> +                                                                       aDoc);

over 80 columns

@@ +1696,5 @@
>    }
>  
>    if (tag == nsGkAtoms::progress) {
>      nsAccessible* accessible =
> +      new HTMLProgressMeterAccessible(aContent, aDoc);

already on one line.

@@ +1794,5 @@
>  #ifdef MOZ_XUL
>        if (parentContent->NodeInfo()->Equals(nsGkAtoms::tabpanels,
>                                              kNameSpaceID_XUL)) {
>          nsAccessible* accessible = new nsXULTabpanelAccessible(aContent,
> +                                                               aDoc);

done

::: accessible/src/base/nsAccessible.cpp
@@ +779,5 @@
>  
>    // Get accessible for the node with the point or the first accessible in
>    // the DOM parent chain.
>    nsAccessible* accessible =
> +    GetAccService()->GetAccessibleOrContainer(content);

Changed back, but passing the presShell instead.

@@ +966,5 @@
>  
>    if (IsDefunct())
>      return NS_ERROR_FAILURE;
>  
> +  nsDocAccessible* doc = GetDocAccessible();

done

@@ +967,5 @@
>    if (IsDefunct())
>      return NS_ERROR_FAILURE;
>  
> +  nsDocAccessible* doc = GetDocAccessible();
> +  nsCOMPtr<nsIPresShell> presShell = doc->GetPresShell();

done

@@ +1647,5 @@
>  
>    // Check if it's a simple xlink.
>    if (nsCoreUtils::IsXLink(mContent)) {
> +    nsDocAccessible* doc = GetDocAccessible();
> +    nsCOMPtr<nsIPresShell> presShell = doc->GetPresShell();

done

@@ +2174,5 @@
>      return;
>  
> +  nsDocAccessible* doc = GetDocAccessible();
> +  if (!doc)
> +    return;

removed. IsDefunct() is enough.

@@ +2176,5 @@
> +  nsDocAccessible* doc = GetDocAccessible();
> +  if (!doc)
> +    return;
> +
> +  nsCOMPtr<nsIPresShell> presShell = doc->GetPresShell();

done

@@ +2958,5 @@
>  nsAccessible::CacheChildren()
>  {
> +  nsDocAccessible* doc = GetDocAccessible();
> +  if(!doc)
> +    return;

done

@@ +3041,2 @@
>    nsAccessible* accessible =
> +    GetAccService()->GetAccessibleInWeakShell(aStartNode, weakShell);

Replacing with a call to GetAccessible()

::: accessible/src/base/nsDocAccessible.h
@@ +137,5 @@
> +  /**
> +   * Return presentation shell for the accessible.
> +   */
> +  nsIWeakReference* GetWeakShell() const
> +    { return mWeakShell; }

done

@@ +171,5 @@
>    /**
>     * Return true if the document has given document state.
>     */
>    bool HasLoadState(LoadState aState) const
> +    { return (mLoadState & (PRUint32)aState) == (PRUint32)aState; }

done

::: accessible/src/base/nsRootAccessible.cpp
@@ +376,5 @@
>      return;
>    }
>  
>    nsAccessible* accessible =
> +    GetAccService()->GetAccessibleOrContainer(origTargetNode);

ok

@@ +562,5 @@
>  void
>  nsRootAccessible::Shutdown()
>  {
>    // Called manually or by nsAccessNode::LastRelease()
> +  if (!mDoc)

With the change above it would be mPresShell. done.

::: accessible/src/html/nsHTMLLinkAccessible.cpp
@@ +118,5 @@
>      return NS_OK;
>    
> +  if (!mDoc)
> +    return NS_ERROR_FAILURE;
> +

done

::: accessible/src/html/nsHTMLSelectAccessible.cpp
@@ +179,5 @@
>  void
>  nsHTMLSelectListAccessible::CacheOptSiblings(nsIContent *aParentContent)
>  {
> +  if (!mDoc)
> +    return;

done

@@ +181,5 @@
>  {
> +  if (!mDoc)
> +    return;
> +
> +  nsCOMPtr<nsIPresShell> presShell(mDoc->GetPresShell());

using a nsIPresShell* instead

::: accessible/src/html/nsHTMLTableAccessible.cpp
@@ +347,5 @@
>      else if (aRowOrColumnHeaderCell == nsAccUtils::eColumnHeaderCells)
>        desiredRole = roles::COLUMNHEADER;
>  
> +    if (!mDoc)
> +      return NS_ERROR_FAILURE;

done

@@ +463,5 @@
>    // caption only, because nsAccessibilityService ensures we don't create
>    // accessibles for the other captions, since only the first is actually
>    // visible.
> +  if(!mDoc)
> +    return;

done

@@ +711,5 @@
>      rowSpan, colSpan, actualRowSpan, actualColSpan;
>    bool isSelected = false;
>  
> +  if (!mDoc)
> +    return NS_ERROR_FAILURE;

done

@@ +1205,5 @@
>      rv = GetRowCount(&count);
>  
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  NS_ENSURE_TRUE(mDoc, NS_ERROR_FAILURE);

done

@@ +1238,5 @@
>  {
>    nsITableLayout *tableLayout = GetTableLayout();
>    NS_ENSURE_STATE(tableLayout);
>  
> +  NS_ENSURE_TRUE(mDoc, NS_ERROR_FAILURE);

done

::: accessible/src/html/nsHTMLTableAccessible.h
@@ +211,5 @@
>  class nsHTMLCaptionAccessible : public nsHyperTextAccessibleWrap
>  {
>  public:
> +  nsHTMLCaptionAccessible(nsIContent* aContent, nsDocAccessible* aDoc) :
> +    nsHyperTextAccessibleWrap(aContent, aDoc) { }

I can, but it is implicit if one of the parent class has one.

::: accessible/src/html/nsHyperTextAccessible.cpp
@@ +1551,5 @@
>    if (!editingSession)
>      return NS_OK; // No editing session interface
>  
> +  NS_ENSURE_TRUE(mDoc, NS_ERROR_FAILURE);
> +  nsCOMPtr<nsIDocument> doc = mDoc->GetDocumentNode();

using nsIDocument* instead

::: accessible/src/html/nsHyperTextAccessible.h
@@ +75,5 @@
>                                public nsIAccessibleHyperText,
>                                public nsIAccessibleEditableText
>  {
>  public:
> +  nsHyperTextAccessible(nsIContent* aContent, nsDocAccessible* aDoc);

empty line added.
virtuall dtor not indeed as empty implicit is since parent class has one.

::: accessible/src/mac/nsAccessNodeWrap.h
@@ +47,5 @@
>  
>  class nsAccessNodeWrap :  public nsAccessNode
>  {
>    public: // construction, destruction
> +    nsAccessNodeWrap(nsIContent* aContent, nsDocAccessible* aShell);

done

::: accessible/src/msaa/nsAccessNodeWrap.cpp
@@ +77,5 @@
>  // nsAccessNodeWrap
>  ////////////////////////////////////////////////////////////////////////////////
>  
>  nsAccessNodeWrap::
> +  nsAccessNodeWrap(nsIContent *aContent, nsDocAccessible* aDoc) :

done

@@ +421,5 @@
>    nsAccessNodeWrap *newNode = NULL;
>  
>    ISimpleDOMNode *iNode = NULL;
> +  nsAccessible *acc = mDoc ?
> +    GetAccService()->GetAccessibleInWeakShell(aNode, mDoc->GetWeakShell()) : nsnull;

done

::: accessible/src/msaa/nsAccessibleWrap.cpp
@@ +1549,5 @@
>    if (!winEvent)
>      return NS_OK;
>  
>    // Means we're not active.
> +  NS_ENSURE_TRUE(mDoc, NS_ERROR_FAILURE);

done

::: accessible/src/xforms/nsXFormsAccessible.cpp
@@ +77,5 @@
>  // nsXFormsAccessible
>  ////////////////////////////////////////////////////////////////////////////////
>  
>  nsXFormsAccessible::
> +nsXFormsAccessible(nsIContent* aContent, nsDocAccessible* aDoc) :

done

@@ +125,1 @@
>      return;

done
(In reply to Hub Figuiere [:hub] from comment #14)

> ::: accessible/src/atk/nsAccessibleWrap.h
> >  class nsAccessibleWrap: public nsAccessible
> >  {
> >  public:
> > +    nsAccessibleWrap(nsIContent* aContent, nsDocAccessible* aDoc);
> 
> The only reason it is not tow spaces is to keep the indentation consistent
> (it is at 4 in there) and not re-indent the whole class declaration.

right, but usually we do that. we get ugly styling for a while but that allows us to end up with unique style

> >  NotificationController::CreateTextChangeEventFor(AccMutationEvent* aEvent)
> >  {
> >    nsAccessible* container =
> > +    GetAccService()->GetContainerAccessible(aEvent->mNode);
> 
> GetContainerAccessible does not use the weakshell it receive so I removed
> it. Maybe I was over enthusiastic.

You're right that nsAccessibilityService::GetCOntainerAccessible() and other methods dont' use presshell information. But we're going to support multipresshel per DOM document so we shouldn't get rid infrastructure for that. Here you can get document accessible from event and ask it for container accessible for the given DOM node.


> ::: accessible/src/base/nsAccUtils.cpp
> @@ +524,5 @@
> >  nsIntPoint
> >  nsAccUtils::GetScreenCoordsForParent(nsAccessNode *aAccessNode)
> >  {
> >    nsAccessible *parent =
> > +    GetAccService()->GetContainerAccessible(aAccessNode->GetNode());
> 
> changed back to what it was before, but we pass the presShell, not the weak
> shell.

> ::: accessible/src/base/nsAccessNode.cpp
> @@ +94,5 @@
> >    NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIAccessNode)
> >  NS_INTERFACE_MAP_END
> >   
> >  NS_IMPL_CYCLE_COLLECTING_ADDREF(nsAccessNode)
> > +NS_IMPL_CYCLE_COLLECTING_RELEASE(nsAccessNode)
> 
> It was used to release the presShell. Since it is not longer in there, it is
> no longer needed and I let the standard implementation instead.

It makes sense to call shutdown on destruction anyway so we are guaranteed we do shutdown properly. At least I wouldn't change that here.

> > +  nsDocAccessible* doc = GetDocAccessible();
> > +  NS_ENSURE_TRUE(doc, NS_ERROR_FAILURE);
> > +
> > +  nsCOMPtr<nsIPresShell> shell(doc->GetPresShell());
> 
> added a dont_AddRef()

you can do nsIPresShell* shell = doc->GetPresShell()

> > +  nsDocAccessible* GetDocAccessible() const { return mDoc; }
> 
> see bug 673405 and how I was proposing to fold it into this one. Because it
> does that change too.

ok, got it

> >  nsAccessibilityService::CreateOuterDocAccessible(nsIContent* aContent,
> >                                                   nsIPresShell* aPresShell)
> >  {
> >    nsCOMPtr<nsIWeakReference> weakShell(do_GetWeakReference(aPresShell));
> > +  nsAccessible* accessible = new nsOuterDocAccessible(aContent, nsAccUtils::GetDocAccessibleFor(weakShell));
> 
> I still don't get what the issue here? We want to not have to create a
> weakShell and just use the presShell?

yep, we don't need weak shells anymore, we can use presshell instead

> @@ +1617,5 @@
> >    }
> >  
> >    if (tag == nsGkAtoms::optgroup) {
> >      nsAccessible* accessible = new nsHTMLSelectOptGroupAccessible(aContent,
> > +                                                                  aDoc);
> 
> Here it is over 80 column (Trevor asked me to wrap at 80)

ok, got it

> >  class nsHTMLCaptionAccessible : public nsHyperTextAccessibleWrap
> >  {
> >  public:
> > +  nsHTMLCaptionAccessible(nsIContent* aContent, nsDocAccessible* aDoc) :
> > +    nsHyperTextAccessibleWrap(aContent, aDoc) { }
> 
> I can, but it is implicit if one of the parent class has one.

I would keep them explicit, we do that in other places.
(In reply to alexander :surkov from comment #15)

> > The only reason it is not tow spaces is to keep the indentation consistent
> > (it is at 4 in there) and not re-indent the whole class declaration.
> 
> right, but usually we do that. we get ugly styling for a while but that
> allows us to end up with unique style

ok then.

> 
> > >  NotificationController::CreateTextChangeEventFor(AccMutationEvent* aEvent)
> > >  {
> > >    nsAccessible* container =
> > > +    GetAccService()->GetContainerAccessible(aEvent->mNode);
> > 
> > GetContainerAccessible does not use the weakshell it receive so I removed
> > it. Maybe I was over enthusiastic.
> 
> You're right that nsAccessibilityService::GetCOntainerAccessible() and other
> methods dont' use presshell information. But we're going to support
> multipresshel per DOM document so we shouldn't get rid infrastructure for
> that. Here you can get document accessible from event and ask it for
> container accessible for the given DOM node.

After other changes I ended up passing the presshell from the document.
Like that aEvent->GetDocAccessible()->PresShell()

> > ::: accessible/src/base/nsAccessNode.cpp
> > @@ +94,5 @@
> > >    NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIAccessNode)
> > >  NS_INTERFACE_MAP_END
> > >   
> > >  NS_IMPL_CYCLE_COLLECTING_ADDREF(nsAccessNode)
> > > +NS_IMPL_CYCLE_COLLECTING_RELEASE(nsAccessNode)
> > 
> > It was used to release the presShell. Since it is not longer in there, it is
> > no longer needed and I let the standard implementation instead.
> 
> It makes sense to call shutdown on destruction anyway so we are guaranteed
> we do shutdown properly. At least I wouldn't change that here.

Ok, I'll back this out.
 
> > > +  nsDocAccessible* doc = GetDocAccessible();
> > > +  NS_ENSURE_TRUE(doc, NS_ERROR_FAILURE);
> > > +
> > > +  nsCOMPtr<nsIPresShell> shell(doc->GetPresShell());
> > 
> > added a dont_AddRef()
> 
> you can do nsIPresShell* shell = doc->GetPresShell()

I ended up doing this in the end.


> > >  nsAccessibilityService::CreateOuterDocAccessible(nsIContent* aContent,
> > >                                                   nsIPresShell* aPresShell)
> > >  {
> > >    nsCOMPtr<nsIWeakReference> weakShell(do_GetWeakReference(aPresShell));
> > > +  nsAccessible* accessible = new nsOuterDocAccessible(aContent, nsAccUtils::GetDocAccessibleFor(weakShell));
> > 
> > I still don't get what the issue here? We want to not have to create a
> > weakShell and just use the presShell?
> 
> yep, we don't need weak shells anymore, we can use presshell instead


ok
 
> > >  class nsHTMLCaptionAccessible : public nsHyperTextAccessibleWrap
> > >  {
> > >  public:
> > > +  nsHTMLCaptionAccessible(nsIContent* aContent, nsDocAccessible* aDoc) :
> > > +    nsHyperTextAccessibleWrap(aContent, aDoc) { }
> > 
> > I can, but it is implicit if one of the parent class has one.
> 
> I would keep them explicit, we do that in other places.

ok
Attachment #591646 - Attachment is obsolete: true
Comment on attachment 592255 [details] [diff] [review]
Don't keep pointer to weak presshell in accessible.

This should address all the comments.

I think now I delete more lines than I add :-)
Attachment #592255 - Flags: review?(surkov.alexander)
Comment on attachment 592255 [details] [diff] [review]
Don't keep pointer to weak presshell in accessible.

Review of attachment 592255 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comments fixed

::: accessible/src/atk/nsAccessibleWrap.cpp
@@ +362,5 @@
>      *aOutAccessible = nsnull;
>  
>      if (!mAtkObject) {
> +        nsDocAccessible* doc = GetDocAccessible();
> +        if (IsDefunct() || !nsAccUtils::IsEmbeddedObject(this)) {

you don't need document accesssible here, right?

::: accessible/src/base/nsAccUtils.cpp
@@ +526,5 @@
>  nsAccUtils::GetScreenCoordsForParent(nsAccessNode *aAccessNode)
>  {
>    nsAccessible *parent =
>      GetAccService()->GetContainerAccessible(aAccessNode->GetNode(),
> +                                            aAccessNode->GetDocAccessible()->PresShell());

nsDocAccessible* document = aAccessNode->GetDocAccessible();
nsAccessible* parent = document->GetContainerAccessible(aAccessNode->GetNode());

::: accessible/src/base/nsAccessNode.cpp
@@ +235,2 @@
>  
> +  nsCOMPtr<nsIPresShell> presShell(mDoc->PresShell());

you don't need to addref it

@@ +235,5 @@
>  
> +  nsCOMPtr<nsIPresShell> presShell(mDoc->PresShell());
> +
> +  // XXX this does not work if there is multiple presshell per document.
> +  return presShell ? presShell->GetPresContext() : nsnull;

nsDocAccessible::PresShell() should take care it returns correct presshell for the document so I'd say you don't need XXX comment here

@@ +328,5 @@
>  
>    nsIFrame *frame = GetFrame();
>    NS_ENSURE_TRUE(frame, NS_ERROR_FAILURE);
>  
> +  nsCOMPtr<nsIContent> content = dont_AddRef(frame->GetContent());

nsIContent* content = frame->GetContent();

::: accessible/src/base/nsAccessibilityService.cpp
@@ +208,5 @@
>  already_AddRefed<nsAccessible>
>  nsAccessibilityService::CreateOuterDocAccessible(nsIContent* aContent,
>                                                   nsIPresShell* aPresShell)
>  {
> +  nsAccessible* accessible = new nsOuterDocAccessible(aContent, nsAccUtils::GetDocAccessibleFor(aPresShell));

nit: > 80 chars
you can do:
nsAccessible* accessible =
  new nsOuterDocAccessible(aContent,
                           nsAccUtils::GetDocAccessibleFor(aPresShell));

@@ +213,1 @@
>    NS_IF_ADDREF(accessible);

btw, while you're here you can do NS_ADDREF because new operator doesn't fail

@@ +222,1 @@
>    NS_IF_ADDREF(accessible);

same and below in this file

@@ +871,5 @@
>    if (!aNode)
>      return nsnull;
>  
>    // XXX: weak shell is ignored until multiple shell documents are supported.
>    nsDocAccessible* document = GetDocAccessible(aNode->OwnerDoc());

use nsAccUtils::GetDocAccessibleFor(aPresShell)) for consistence and then remove XXX comment

::: accessible/src/base/nsAccessible.cpp
@@ +2950,5 @@
>  nsAccessible::CacheChildren()
>  {
> +  nsDocAccessible* doc = GetDocAccessible();
> +  if(!doc)
> +    return;

that's something that should never happen. Please add assertion if you think it makes sense to keep this check

@@ +3025,5 @@
>  
>  nsAccessible *
>  nsAccessible::GetFirstAvailableAccessible(nsINode *aStartNode) const
>  {
> +  nsAccessible* accessible = GetAccService()->GetAccessible(aStartNode);

we used here mWeakShell so now we should use document like
mDoc->GetAccessible(aStartNode);

@@ +3047,5 @@
>      if (!currentNode)
>        return nsnull;
>  
>      nsCOMPtr<nsINode> node(do_QueryInterface(currentNode));
> +    nsAccessible* accessible = GetAccService()->GetAccessible(node);

same here

::: accessible/src/base/nsDocAccessible.cpp
@@ +102,5 @@
>  
>  nsDocAccessible::
>    nsDocAccessible(nsIDocument *aDocument, nsIContent *aRootContent,
>                    nsIWeakReference *aShell) :
> +  nsHyperTextAccessibleWrap(aRootContent, this),

is it safe to pass this before we constructed it? All we need is just to make sure it's not used in base class constructor, correct?

@@ +202,5 @@
> +    NS_ASSERTION(!mPresShell, "A Shutdown() impl forgot to call its parent's Shutdown?");
> +  }
> +  // ... then die.
> +  delete this;
> +}

so you get back LastRelease() in nsAccessNode, why do you need this one?

::: accessible/src/base/nsDocAccessible.h
@@ +129,5 @@
>  
>    // nsDocAccessible
>  
>    /**
> +   * Return the corresponding press shell for this accessible.

maybe: Return presentation shell for this document accessible.

@@ +134,5 @@
> +   */
> +  nsIPresShell* PresShell() const { return mPresShell; }
> +
> +  /**
> +   * Return presentation shell for the accessible.

maybe: Return weak reference to presentation shell for this document accessible.
please keep similar comments for these two methods

@@ +650,5 @@
>    friend class NotificationController;
> +
> +private:
> +
> +  nsCOMPtr<nsIPresShell> mPresShell;

you don't need to keep strong reference to presshell because presshell notifies the document when it goes away

::: accessible/src/html/nsHyperTextAccessible.cpp
@@ +1551,5 @@
>    if (!editingSession)
>      return NS_OK; // No editing session interface
>  
> +  NS_ENSURE_TRUE(mDoc, NS_ERROR_FAILURE);
> +  nsIDocument* doc = mDoc->GetDocumentNode();

could you please change 'doc' to documentNode or docNode?

@@ +1594,5 @@
>    // Now that selection is done, move the focus to the selection.
>    nsFocusManager* DOMFocusManager = nsFocusManager::GetFocusManager();
>    if (DOMFocusManager) {
> +    NS_ENSURE_TRUE(mDoc, NS_ERROR_FAILURE);
> +    nsCOMPtr<nsIDocument> doc = mDoc->GetDocumentNode();

you don't need to addref it, please fix while you're here
could you please change 'doc' to documentNode or docNode?

::: accessible/src/mac/nsAccessNodeWrap.h
@@ +46,5 @@
>  #include "nsAccessNode.h"
>  
>  class nsAccessNodeWrap :  public nsAccessNode
>  {
> +public: // construction, destruction

// construction, desctruction is not so valuable comment, you could remove it

::: accessible/src/msaa/nsAccessNodeWrap.cpp
@@ +421,5 @@
>    nsAccessNodeWrap *newNode = NULL;
>  
>    ISimpleDOMNode *iNode = NULL;
> +  nsAccessible* acc = mDoc ?
> +    GetAccService()->GetAccessibleInWeakShell(aNode, mDoc->GetWeakShell()) : nsnull;

just do
nsAccessible* acc = mDoc->GetAccessible(aNode);

::: accessible/src/msaa/nsHTMLWin32ObjectAccessible.h
@@ +53,5 @@
>    // because the native plugin accessible doesn't know where it exists in the
>    // Mozilla tree, and returns null for previous and next sibling. This would
>    // have the effect of cutting off all content after the plugin.
> +  nsHTMLWin32ObjectOwnerAccessible(nsIContent* aContent,
> +                                   nsDocAccessible* aDoc, void *aHwnd);

void* aHwnd

::: accessible/src/xforms/nsXFormsAccessible.cpp
@@ +357,5 @@
>        return nsnull;
>  
>      nsCOMPtr<nsINode> itemNode(do_QueryInterface(itemDOMNode));
>      nsIAccessible* item = GetAccService()->GetAccessibleInWeakShell(itemNode,
> +                                                                    weakShell);

mDoc->GetAccessible(itemNode)

@@ +446,5 @@
>      rv = sXFormsService->GetSelectedItemForSelect1(DOMNode,
>                                                     getter_AddRefs(itemDOMNode));
>      if (NS_SUCCEEDED(rv) && itemDOMNode) {
>        nsCOMPtr<nsINode> itemNode(do_QueryInterface(itemDOMNode));
> +      return GetAccService()->GetAccessibleInWeakShell(itemNode, mDoc->GetWeakShell());

same

@@ +461,5 @@
>    nsCOMPtr<nsIDOMNode> itemDOMNode;
>    itemNodeList->Item(aIndex, getter_AddRefs(itemDOMNode));
>  
>    nsCOMPtr<nsINode> itemNode(do_QueryInterface(itemDOMNode));
> +  return GetAccService()->GetAccessibleInWeakShell(itemNode, mDoc->GetWeakShell());

same

::: accessible/src/xforms/nsXFormsWidgetsAccessible.cpp
@@ +112,5 @@
>  // nsXFormsCalendarWidgetAccessible
>  ////////////////////////////////////////////////////////////////////////////////
>  
>  nsXFormsCalendarWidgetAccessible::
> +nsXFormsCalendarWidgetAccessible(nsIContent* aContent, nsDocAccessible* aDoc) :

nit: two spaces indent please

::: accessible/src/xul/XULSelectControlAccessible.cpp
@@ +106,5 @@
>        nsCOMPtr<nsIDOMXULSelectControlItemElement> itemElm;
>        xulMultiSelect->GetSelectedItem(index, getter_AddRefs(itemElm));
>        nsCOMPtr<nsINode> itemNode(do_QueryInterface(itemElm));
>        nsAccessible* item =
> +        GetAccService()->GetAccessibleInWeakShell(itemNode, weakShell);

mDoc->GetAccessible(itemNode);

@@ +117,5 @@
>        mSelectControl->GetSelectedItem(getter_AddRefs(itemElm));
>        nsCOMPtr<nsINode> itemNode(do_QueryInterface(itemElm));
>        if(itemNode) {
>          nsAccessible* item =
> +          GetAccService()->GetAccessibleInWeakShell(itemNode, weakShell);

same

@@ +143,5 @@
>      mSelectControl->GetSelectedItem(getter_AddRefs(itemElm));
>  
>    nsCOMPtr<nsINode> itemNode(do_QueryInterface(itemElm));
> +  return itemNode && mDoc ?
> +    GetAccService()->GetAccessibleInWeakShell(itemNode, mDoc->GetWeakShell()) : nsnull;

same

::: accessible/src/xul/nsXULColorPickerAccessible.cpp
@@ +177,5 @@
>  void
>  nsXULColorPickerAccessible::CacheChildren()
>  {
> +  if (!mDoc)
> +    return;

assertion

::: accessible/src/xul/nsXULComboboxAccessible.cpp
@@ +136,2 @@
>      nsAccessible* focusedOptionAcc = GetAccService()->
> +      GetAccessibleInWeakShell(focusedOptionContent, mDoc->GetWeakShell());

mDoc->GetAccessible

::: accessible/src/xul/nsXULFormControlAccessible.cpp
@@ +230,1 @@
>      return;

I'd assert for no document because that's something really bad

@@ +874,5 @@
>  {
>    // Create child accessibles for native anonymous content of underlying HTML
>    // input element.
>    nsCOMPtr<nsIContent> inputContent(GetInputField());
> +  if (!inputContent || !mDoc)

same here

::: accessible/src/xul/nsXULListboxAccessible.cpp
@@ +321,2 @@
>    nsAccessible *row =
> +    GetAccService()->GetAccessibleInWeakShell(itemContent, mDoc->GetWeakShell());

mDoc->GetAccessible(

@@ +609,5 @@
>      nsCOMPtr<nsIDOMNode> itemNode;
>      selectedItems->Item(index, getter_AddRefs(itemNode));
>      nsCOMPtr<nsIContent> itemContent(do_QueryInterface(itemNode));
>      nsAccessible *item =
> +      GetAccService()->GetAccessibleInWeakShell(itemContent, weakShell);

same

@@ +921,1 @@
>      return nsnull;

I think I would move mDoc check into IsDefunct()

@@ +931,5 @@
>    nsCOMPtr<nsIContent> listContent(do_QueryInterface(list));
>    if (!listContent)
>      return nsnull;
>  
> +  return GetAccService()->GetAccessibleInWeakShell(listContent, mDoc->GetWeakShell());

mDoc->GetAccessible

::: accessible/src/xul/nsXULTreeGridAccessible.cpp
@@ +1077,5 @@
>    mColumn->GetElement(getter_AddRefs(columnElm));
>  
>    nsCOMPtr<nsIContent> columnContent(do_QueryInterface(columnElm));
>    nsAccessible *headerCell =
> +    GetAccService()->GetAccessibleInWeakShell(columnContent, mDoc->GetWeakShell());

mDoc->GetAccessible
Attachment #592255 - Flags: review?(surkov.alexander) → review+
Blocks: 721947
Comment on attachment 592255 [details] [diff] [review]
Don't keep pointer to weak presshell in accessible.

Review of attachment 592255 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/atk/nsAccessibleWrap.cpp
@@ +362,5 @@
>      *aOutAccessible = nsnull;
>  
>      if (!mAtkObject) {
> +        nsDocAccessible* doc = GetDocAccessible();
> +        if (IsDefunct() || !nsAccUtils::IsEmbeddedObject(this)) {

indeed. I no longer need.

::: accessible/src/base/nsDocAccessible.cpp
@@ +102,5 @@
>  
>  nsDocAccessible::
>    nsDocAccessible(nsIDocument *aDocument, nsIContent *aRootContent,
>                    nsIWeakReference *aShell) :
> +  nsHyperTextAccessibleWrap(aRootContent, this),

The pointer is safe. The object pointed to isn't, but I checked and we don't use the object in any constructor. Just keep the pointer around.

@@ +202,5 @@
> +    NS_ASSERTION(!mPresShell, "A Shutdown() impl forgot to call its parent's Shutdown?");
> +  }
> +  // ... then die.
> +  delete this;
> +}

oops. removing.

::: accessible/src/xul/nsXULListboxAccessible.cpp
@@ +921,1 @@
>      return nsnull;

actually it is already done there. I forgot to remove that check.
Target Milestone: --- → mozilla13
The orange was caused by nsAccessibilityService::GetAccessibleOrContainer() being passed a nsnull presShell. Since it used the presShell to get the docAccessible I'm changing it to actually get a docAccessible passed, and with a fallback if it is nsnull to get the nsDocAccessible from the owner document.

I'll repost the new patch with these changes once I get the try build confirmed.
Comment on attachment 594916 [details] [diff] [review]
Don't keep pointer to weak presshell in accessible. v2

This version fixes the orange. Brand new patch.
Attachment #594916 - Attachment description: Don't keep pointer to weak presshell in accessible. → Don't keep pointer to weak presshell in accessible. v2
Attachment #594916 - Flags: review?(surkov.alexander)
Comment on attachment 594916 [details] [diff] [review]
Don't keep pointer to weak presshell in accessible. v2

Review of attachment 594916 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

::: accessible/src/base/nsAccUtils.cpp
@@ +428,4 @@
>  
>    // Get text accessible containing the result node.
>    nsAccessible* accessible =
> +    GetAccService()->GetAccessibleOrContainer(resultNode, nsnull);

do document guess here

::: accessible/src/base/nsAccessibilityService.cpp
@@ +936,5 @@
>    if (!aNode)
>      return nsnull;
>  
> +  if (!aDoc)
> +    aDoc = GetDocAccessible(aNode->OwnerDoc());

fail if no document, caller should pass not null document

::: accessible/src/base/nsAccessibilityService.h
@@ +224,5 @@
>  
>    /**
>     * Return a container accessible for the given DOM node.
>     */
> +  inline nsAccessible* GetContainerAccessible(nsINode* aNode, nsDocAccessible* aDoc)

you could remove 'inline' keyword while you're here since it's not necessary, right?
Attachment #594916 - Flags: review?(surkov.alexander) → review+
Backed out of inbound for mochitest-other leaks:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=0b205d34aefc
{
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 9024 bytes during test execution
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 282 instances of nsWeakReference with size 32 bytes each (9024 bytes total)
}

https://hg.mozilla.org/integration/mozilla-inbound/rev/3e7875d6ee51
Target Milestone: mozilla13 → ---
apparently the patch for bug 721947 solve the leaks. (we actually leak a IWeakReference)
No longer blocks: 721947
Depends on: 721947
(In reply to Hub Figuiere [:hub] from comment #29)
> apparently the patch for bug 721947 solve the leaks. (we actually leak a
> IWeakReference)

you could check in these as single changeset or port the code here that fix a leak.
philor suggested I push them all 3 together since they don't turn the tree orange on leaks. About to submit it for review.
(In reply to Hub Figuiere [:hub] from comment #31)
> philor suggested I push them all 3 together since they don't turn the tree
> orange on leaks. About to submit it for review.

why do you need review in this case?
for bug 721947 I'll need a review
https://hg.mozilla.org/mozilla-central/rev/1e44a27c3b6b
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.