809871 - XUL tree accessible creation may flush layout

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Stack:

 	xul.dll!PresShell::FlushPendingNotifications(mozFlushType aType)  Line 3806 + 0x2d bytes	C++
 	xul.dll!nsBoxObject::GetFrame(bool aFlushLayout)  Line 126	C++
 	xul.dll!nsTreeBoxObject::GetTreeBody(bool aFlushLayout)  Line 103 + 0x10 bytes	C++
 	xul.dll!nsTreeBoxObject::GetColumns(nsITreeColumns * * aColumns)  Line 217 + 0xa bytes	C++
>	xul.dll!nsAccessibilityService::CreateAccessibleForXULTree(nsIContent * aContent, DocAccessible * aDoc)  Line 1650 + 0x31 bytes	C++
 	xul.dll!nsAccessibilityService::CreateAccessibleByType(nsIContent * aContent, DocAccessible * aDoc)  Line 1225 + 0x17 bytes	C++
 	xul.dll!nsAccessibilityService::GetOrCreateAccessible(nsINode * aNode, DocAccessible * aDoc, bool * aIsSubtreeHidden)  Line 935 + 0x1a bytes	C++
 	xul.dll!nsAccTreeWalker::NextChildInternal(bool aNoWalkUp)  Line 82 + 0x3a bytes	C++
 	xul.dll!nsAccTreeWalker::NextChildInternal(bool aNoWalkUp)  Line 92 + 0xa bytes	C++
 	xul.dll!nsAccTreeWalker::NextChildInternal(bool aNoWalkUp)  Line 92 + 0xa bytes	C++
 	xul.dll!nsAccTreeWalker::NextChildInternal(bool aNoWalkUp)  Line 101 + 0x1b bytes	C++
 	xul.dll!nsAccTreeWalker::NextChildInternal(bool aNoWalkUp)  Line 101 + 0x1b bytes	C++
 	xul.dll!nsAccTreeWalker::NextChild()  Line 36	C++
 	xul.dll!Accessible::CacheChildren()  Line 2937 + 0x8 bytes	C++
 	xul.dll!Accessible::EnsureChildren()  Line 2979	C++
 	xul.dll!Accessible::UpdateChildren()  Line 324	C++
 	xul.dll!DocAccessible::ProcessContentInserted(Accessible * aContainer, const nsTArray<nsCOMPtr<nsIContent>,nsTArrayDefaultAllocator> * aInsertedContent)  Line 1826	C++
 	xul.dll!NotificationController::ContentInsertion::Process()  Line 901	C++
 	xul.dll!NotificationController::WillRefresh(mozilla::TimeStamp aTime)  Line 231	C++

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

this fixes *this* issue but probably we don't need to flash layout when we operate on XUL tree at all

Comment 2

[alexander :surkov (:asurkov)]

Comment on attachment 679692 [details] [diff] [review]
patch

it doesn't help (per chat with Trev on irc)

Comment 3

[alexander :surkov (:asurkov)]

if there's no cached treeBodyFrame then layout will be flushed unconditionally:

either by GetPresShell in nsBoxObject::GetFrame
http://mxr.mozilla.org/mozilla-central/source/layout/xul/base/src/nsBoxObject.cpp#145

or by nsBoxObject::GetFrame
http://mxr.mozilla.org/mozilla-central/source/layout/xul/base/src/nsBoxObject.cpp#117

cc'ing Boris for ideas.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Working with box objects will flush layout, yes...

But flushing layout from inside WillRefresh is ok, in general.  What problem are we trying to solve?

Comment 5

[alexander :surkov (:asurkov)]

At that point we create accessible and if we trigger a layout flush then frame tree can be changed. iirc we had crashes because of that in the past.

here's some background: bug 629912 comment #36

Comment 6

[alexander :surkov (:asurkov)]

btw, assertion was introduced in bug 634197

Comment 7

[Boris Zbarsky [:bzbarsky]]

OK.  Sounds like you should do whatever needs a box object off a script runner....

Comment 8

[alexander :surkov (:asurkov)]

How can I do that, any hint, example please?

Comment 9

[Boris Zbarsky [:bzbarsky]]

nsContentUtils::AddScriptRunner.  It'll make sure to run the runnable as soon as it's safe in the sense that there are no more scriptblockers on the stack.

Then you put a scriptblocker on the stack while you're doing stuff you don't want interrupted.

Comment 10

[Trevor Saunders (:tbsaunde)]

(In reply to Boris Zbarsky (:bz) from comment #7)
> OK.  Sounds like you should do whatever needs a box object off a script
> runner....

Well, so that's tricky because a lot of this stuff wants things to happen sync.  also it means we'll block assistive technology on our main loop (right?)  which sounds really bad for there perf, and actually sounds like it might be able to lead to some sort of deadlock with the assistive technology especially in the wonderful land of win32 :-(

I suspect without checking we don't really want the box object as much as we want the tree frame / view / columns, and just go through the tree box object to get them.

Comment 11

[Boris Zbarsky [:bzbarsky]]

> because a lot of this stuff wants things to happen sync. 

How sync?

> also it means we'll block assistive technology on our main loop (right?)

No.  What I proposed is not dispatching events to the event loop.  Just queuing them up until a stack unwinds.

> as much as we want the tree frame / view / columns

Right, but getting those generally involves a flush, if you want up-to-date information...

Comment 12

[Trevor Saunders (:tbsaunde)]

(In reply to Boris Zbarsky (:bz) from comment #11)
> > because a lot of this stuff wants things to happen sync. 
> 
> How sync?

for example we could be implementing a method like this

unsigned int
Tree:::RowCount()
{
....
}

which needs the result of what we do with the tree body.

Such a function would be to implement say a IA2 interface which other apps can call.

> > also it means we'll block assistive technology on our main loop (right?)
> 
> No.  What I proposed is not dispatching events to the event loop.  Just
> queuing them up until a stack unwinds.

so, we use box objects in two fiarly seperate situations

first we have constructed accessible tree, and external thing calls into us asking for information about tree, say how many rows it has, then the stack looks like this.

stuff fiddling with xul tree
Tree::RowCount()
windowsTableWrapper::get_rowCount()
external goop

so unwinding is um tricky if I understand you write

the other case this bug was actually filed for is  the case where we're building the accessible tree in the WillRefresh callback, there we're using nsAccTreeWalker to collect child accessibles and put them in the tree.  the way we use nsAccTreeWalker assumes we can get accessible children in the order they should be children.  So if we get the accessible for the tree later we need to figure out where to insert it, which should be possible for this special case if we can assume no accessible tree mutation between when we dispatch the script runner and when it runs, but its not really clear to me how that could be true since there will be another WillRefresh() between when we dispatch the runner, and when it runs no?

> > as much as we want the tree frame / view / columns
> 
> Right, but getting those generally involves a flush, if you want up-to-date
> information...

so, for the case we are  in the WillRefresh() callback  it shouldn't be out of date right? for the other case I think the "current information that was used to render last time would be just fine since that's what the user sees.  Further I think it would be pretty bad if we flushed in this sort of call because it would mean that we'd do an accessible tree update caused by WillRefresh(), as well as fireing events to whatever external things are listening, so before the call returns to the assistive technology we will have called into them  for events.  Note that when we call into them for events they will often call things like the example RowCount() above so there's a good bit of danger of all sorts of crazy things here.

sorry if that was long but still not clear what the situation here is.

Comment 13

[Boris Zbarsky [:bzbarsky]]

> first we have constructed accessible tree, and external thing calls into us asking for
> information about tree, say how many rows it has, then the stack looks like this.

In this case, flushing layout is OK, right?  Well, apart from the fact that flushing can run script, which can spin nested event loops....

> So if we get the accessible for the tree later

I'd think you get the accessible for the tree the same time.  Just whatever it needs layout info for would happen later.

> since there will be another WillRefresh() between when we dispatch the runner, and when
> it runs no?

There shouldn't be, in general...

> so, for the case we are  in the WillRefresh() callback  it shouldn't be out of date
> right? 

Sure it could be.  WillRefresh comes before processing of pending reflows, since the assumption is that refresh observers might want to change layout.

> further I think it would be pretty bad if we flushed in this sort of call because it
> would mean that we'd do an accessible tree update caused by WillRefresh()

Flushing doesn't call WillRefresh...

> sorry if that was long but still not clear what the situation here is.

That we agree on.  ;)

If stale layout info is ok for your purposes, then don't use box objects.  Those are meant to provide information to JS, which means up-to-date information.

Comment 14

[Trevor Saunders (:tbsaunde)]

(In reply to Boris Zbarsky (:bz) from comment #13)
> > first we have constructed accessible tree, and external thing calls into us asking for
> > information about tree, say how many rows it has, then the stack looks like this.
> 
> In this case, flushing layout is OK, right?  Well, apart from the fact that
> flushing can run script, which can spin nested event loops....

well, if we run a nested event loop then it all depends what it does, but it would seem it could easily do things that would totally break the world.  So first lets just consider what happens if we don't have a nested event loop.  In that case it is  certainly ok to flush layout if we gaurentee that the following things are true.
* refresh observers aren't called while RowCount() is on the stack.
* nsAccessibilitySerivce::ContentRemoved() isn't called while RowCount() is on the stack, this one might be safe, but I'd hate to gamble on that, I'm not sure all of the functions like that behave properly if tree mutation happens during them.

> > So if we get the accessible for the tree later
> 
> I'd think you get the accessible for the tree the same time.  Just whatever
> it needs layout info for would happen later.

well, currently what type of object we create depends on information we get from layout.

> > since there will be another WillRefresh() between when we dispatch the runner, and when
> > it runs no?
> 
> There shouldn't be, in general...
> 
> > so, for the case we are  in the WillRefresh() callback  it shouldn't be out of date
> > right? 
> 
> Sure it could be.  WillRefresh comes before processing of pending reflows,
> since the assumption is that refresh observers might want to change layout.
> > further I think it would be pretty bad if we flushed in this sort of call because it
> > would mean that we'd do an accessible tree update caused by WillRefresh()
> 
> Flushing doesn't call WillRefresh...

ah! sorry! somehow I was under the impression it did.

> > sorry if that was long but still not clear what the situation here is.
> 
> That we agree on.  ;)

sorry again :\

> If stale layout info is ok for your purposes, then don't use box objects. 
> Those are meant to provide information to JS, which means up-to-date
> information.

the way things are now I think that's the right course to take, in general I think a11y should basically never cause layout to be flushed, but I'm ready to be convinced otherwise.

For this case is it ok to get the frame for the tree just with GetPrimaryFrame() on the <tree> node? or is there something special here.  I'm not really clear on why this code ever used boxObjects, or tbh what boxObjects are really for in the first place.

Comment 15

[Boris Zbarsky [:bzbarsky]]

> * refresh observers aren't called while RowCount() is on the stack.

Modulo script running, I believe this will hold true.

> * nsAccessibilitySerivce::ContentRemoved() isn't called while RowCount() is on the stack

This does not hold; elements might pick up a display:none style due to a flush.

> well, currently what type of object we create depends on information we get from layout.

OK.  This affects the object itself, not its descendants?

> sorry again :\

No need to be.  Bug comments are suboptimal in a lot of ways for actually communicating.  :(

> in general I think a11y should basically never cause layout to be flushed

Sounds like a good principle to me!

> For this case is it ok to get the frame for the tree just with GetPrimaryFrame() on the
> <tree> node?

It should be, yes.

> or tbh what boxObjects are really for in the first place.

For nicely packaging up some information that only layout has so that JS can poke at it.

Comment 16

[Trevor Saunders (:tbsaunde)]

(In reply to Boris Zbarsky (:bz) from comment #15)
> > * refresh observers aren't called while RowCount() is on the stack.
> 
> Modulo script running, I believe this will hold true.
> 
> > * nsAccessibilitySerivce::ContentRemoved() isn't called while RowCount() is on the stack
> 
> This does not hold; elements might pick up a display:none style due to a
> flush.

ok, that's good to know, as below I suspect it doesn't matter for right know though since we should rewrite a11y to not use box objects.

> > For this case is it ok to get the frame for the tree just with GetPrimaryFrame() on the
> > <tree> node?
> 
> It should be, yes.

sounds good, I think we should be able to just stop using box objects fairly easily then.

> > or tbh what boxObjects are really for in the first place.
> 
> For nicely packaging up some information that only layout has so that JS can
> poke at it.

ok, thanks

btw if boxes always flush then shouldn't the functions Alex initially linked have there should flush arg removed so this doesn't confuse other people?

Comment 17

[Boris Zbarsky [:bzbarsky]]

Well, they always flush style changes.  The argument controls whether they also flush pending reflows.

Comment 18

[Trevor Saunders (:tbsaunde)]

> > > For this case is it ok to get the frame for the tree just with GetPrimaryFrame() on the
> > > <tree> node?
> > 
> > It should be, yes.
> 
> sounds good, I think we should be able to just stop using box objects fairly
> easily then.

Well, I started looking into doing this (not using box objects for xul trees in a11y, and using tree body frame / tree columns directly), and it seems doable though a bit of work.

Comment 19

[alexander :surkov (:asurkov)]

I'm curious don't we fail to create the tree if we switch to direct frames usage? scenario: there's no frames or they out of date on WillRefresh when we create tree. And then no notifications into a11y to update the tree.

Btw, what's bad in box object usage, say, from API methods?

Comment 20

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #19)
> I'm curious don't we fail to create the tree if we switch to direct frames
> usage? scenario: there's no frames or they out of date on WillRefresh when
> we create tree. And then no notifications into a11y to update the tree.

this seems like question of how abstract nonexisting patch works, so I'm not sure how to answer it usefully.  However I don't see why creating accessible in the case is different from other sorts of accessibles.

> Btw, what's bad in box object usage, say, from API methods?

the API involves flushing layout? ( see last bit of comment 13)

Comment 21

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #20)
> (In reply to alexander :surkov from comment #19)
> > I'm curious don't we fail to create the tree if we switch to direct frames
> > usage? scenario: there's no frames or they out of date on WillRefresh when
> > we create tree. And then no notifications into a11y to update the tree.
> 
> this seems like question of how abstract nonexisting patch works, so I'm not
> sure how to answer it usefully.  However I don't see why creating accessible
> in the case is different from other sorts of accessibles.

It's not a question about abstract patch, it's a question about approach you taken. So if you switch to frame usage then don't we run into problems of tree creation/update?

> > Btw, what's bad in box object usage, say, from API methods?
> 
> the API involves flushing layout? ( see last bit of comment 13)

that bit says we can change if we are ok with out of date information. But the same time I'm sure we are ok with updated information so why would we bother to change it? (Do I miss something?)

Comment 22

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #20)
> However I don't see why creating accessible
> in the case is different from other sorts of accessibles.

I asked because we have bunch of exceptions where generic layout notifications don't work, for example, iirc we have special notifications for XUL menus and XUL listboxes.

Comment 23

[Trevor Saunders (:tbsaunde)]

> > For this case is it ok to get the frame for the tree just with GetPrimaryFrame() on the
> > <tree> node?
> 
> It should be, yes.

so, it seems like I should ask better questions getting the frame for the <tree> gets you a frame but its a nsBoxFrame, you need to get the frame for the <treecols> to get a nsTreeBodyFrame.  That's a tad inconvenient for my purposes, but I can probably work with it.

Comment 24

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #21)
> (In reply to Trevor Saunders (:tbsaunde) from comment #20)
> > (In reply to alexander :surkov from comment #19)
> > > I'm curious don't we fail to create the tree if we switch to direct frames
> > > usage? scenario: there's no frames or they out of date on WillRefresh when
> > > we create tree. And then no notifications into a11y to update the tree.
> > 
> > this seems like question of how abstract nonexisting patch works, so I'm not
> > sure how to answer it usefully.  However I don't see why creating accessible
> > in the case is different from other sorts of accessibles.
> 
> It's not a question about abstract patch, it's a question about approach you
> taken. So if you switch to frame usage then don't we run into problems of
> tree creation/update?
> 
> > > Btw, what's bad in box object usage, say, from API methods?
> > 
> > the API involves flushing layout? ( see last bit of comment 13)
> 
> that bit says we can change if we are ok with out of date information. But
> the same time I'm sure we are ok with updated information so why would we
> bother to change it? (Do I miss something?)

well, I believe getting up to date information requires flushing layout so your options I believe are
1. use existing information and don't flush
2. get get information up to date and in sync with js, but flush.
I'd say 1 is preferable.

Comment 25

[Trevor Saunders (:tbsaunde)]

(In reply to Trevor Saunders (:tbsaunde) from comment #23)
> > > For this case is it ok to get the frame for the tree just with GetPrimaryFrame() on the
> > > <tree> node?
> > 
> > It should be, yes.
> 
> so, it seems like I should ask better questions getting the frame for the
> <tree> gets you a frame but its a nsBoxFrame, you need to get the frame for
> the <treecols> to get a nsTreeBodyFrame.  That's a tad inconvenient for my
> purposes, but I can probably work with it.

err, treechildren not treecols

Comment 26

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #24)

> well, I believe getting up to date information requires flushing layout so
> your options I believe are
> 1. use existing information and don't flush
> 2. get get information up to date and in sync with js, but flush.
> I'd say 1 is preferable.

what's wrong with flushing in general? We shouldn't do that for a11y tree creation for XUL tree but in general it seems ok. Also, I think I would prefer to keep tree creation when information is up to date.

Comment 27

[Trevor Saunders (:tbsaunde)]

Attachment: nsTreeColumns should store nsTreeBodyFrame* not nsTreeBoxObject*

I'm not sure where the tests for this stuff are if there are any good ones, but the atleast passes mochitest-a11y with the next two patches

feel free to redirect these reviews if you like.

Comment 28

[Trevor Saunders (:tbsaunde)]

Attachment: outparamdel some nsTreeBodyFrame methods

just make things a tad nicer.

Comment 29

[Trevor Saunders (:tbsaunde)]

Attachment: wip

I think this avoids calling most of the things during creation that can flush  I think nsTreeBodyFrame::GetView() is the only thing still used that can but that should be easy to fix with a GetExistingView() method.

It'd probably be nice to add a nsCoreUtils method to get the  tree body frame for the <tree> content but I'm not sure about that yet

thoughts?

Comment 30

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 683271 [details] [diff] [review]
nsTreeColumns should store nsTreeBodyFrame* not nsTreeBoxObject*

Looks ok at first glance, but Mats should have a look.

Comment 31

[Mats Palmgren (inactive)]

Comment on attachment 683271 [details] [diff] [review]
nsTreeColumns should store nsTreeBodyFrame* not nsTreeBoxObject*

>layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp
> nsTreeBodyFrame::~nsTreeBodyFrame()
> {
>+  if (mColumns) {
>+    mColumns->SetTree(nullptr);
>+  }

Why? We already do that in DestroyFrom.
Also, there's no need to null-check, right?

>layout/xul/base/src/tree/src/nsTreeColumns.cpp
> NS_IMETHODIMP
> nsTreeColumns::GetTree(nsITreeBoxObject** _retval)
> {
>-  NS_IF_ADDREF(*_retval = mTree);
>+  if (!mTree) {
>+    return NS_OK;
>+  }
>+
>+  NS_IF_ADDREF(*_retval = mTree->GetTreeBoxObject());
>   return NS_OK;
> }

You forgot to assign "*_retval" when !mTree.
I would prefer a one-liner:
NS_IF_ADDREF(*_retval = mTree ? mTree->GetTreeBoxObject() : nullptr);

> nsTreeColumns::RestoreNaturalOrder()

Hmm, this method calls SetAttr with 'true' for aNotify.
Doesn't that mean that the frame, which owns the nsTreeColumns object
can be destroyed? and if so 'this' can be destroyed when we access
'mTree' after the loop, unless the caller holds a strong ref on it...

The outparamdel patch makes that possible, so perhaps Columns() should
return an already_AddRefed<nsTreeColumns> instead, to make it less
error prone?

Comment 32

[Mats Palmgren (inactive)]

Comment on attachment 683272 [details] [diff] [review]
outparamdel some nsTreeBodyFrame methods

>layout/xul/base/src/tree/src/nsTreeBodyFrame.h
>+  nsTreeColumns* Columns() const
>+  {
>+    return mColumns;
>+  }

Style nit: the prevailing brace style in this file is hanging braces, or
on the same line if the body fits.  It looks like all the methods you
add is <= 80 columns, so I'd prefer this style:
  nsTreeColumns* Columns() const { return mColumns; }
etc.

r+ with that nit fixed , but I'd like to review it again if you change
the Columns() return type per last comment.

Comment 33

[Trevor Saunders (:tbsaunde)]

(In reply to Mats Palmgren [:mats] from comment #31)
> Comment on attachment 683271 [details] [diff] [review]
> nsTreeColumns should store nsTreeBodyFrame* not nsTreeBoxObject*
> 
> >layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp
> > nsTreeBodyFrame::~nsTreeBodyFrame()
> > {
> >+  if (mColumns) {
> >+    mColumns->SetTree(nullptr);
> >+  }
> 
> Why? We already do that in DestroyFrom.

do we always call DestroyFrom() before deleting frames? (I have no idea)

> Also, there's no need to null-check, right?

I can't think of one...

> >layout/xul/base/src/tree/src/nsTreeColumns.cpp
> > NS_IMETHODIMP
> > nsTreeColumns::GetTree(nsITreeBoxObject** _retval)
> > {
> >-  NS_IF_ADDREF(*_retval = mTree);
> >+  if (!mTree) {
> >+    return NS_OK;
> >+  }
> >+
> >+  NS_IF_ADDREF(*_retval = mTree->GetTreeBoxObject());
> >   return NS_OK;
> > }
> 
> You forgot to assign "*_retval" when !mTree.
> I would prefer a one-liner:
> NS_IF_ADDREF(*_retval = mTree ? mTree->GetTreeBoxObject() : nullptr);

yeah, I must have been out of it or something :(

> > nsTreeColumns::RestoreNaturalOrder()
> 
> Hmm, this method calls SetAttr with 'true' for aNotify.
> Doesn't that mean that the frame, which owns the nsTreeColumns object
> can be destroyed? and if so 'this' can be destroyed when we access
> 'mTree' after the loop, unless the caller holds a strong ref on it...
> 
> The outparamdel patch makes that possible, so perhaps Columns() should
> return an already_AddRefed<nsTreeColumns> instead, to make it less
> error prone?

well, that method's name indicates fairly strongly it mutates stuff, and not holding a ref when making the call breaks xpcom rules, but I see your point.  On the other hand I sort of hate to add the refcounting.

Comment 34

[Mats Palmgren (inactive)]

> do we always call DestroyFrom() before deleting frames? (I have no idea)

Yes, it's DestroyFrom() that calls the destructor:
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#680
and "delete aFrame" is forbidden:
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#403

> well, that method's name indicates fairly strongly it mutates stuff

I don't see why Columns() would suggest that.  We have this naming
convention in Layout that we only prefix an accessor with Get if it
may return null, otherwise we skip that.  As far as I know, that
naming convention applies to all of Gecko (for C++).

> and not holding a ref when making the call breaks xpcom rules

Since the caller is required to hold a strong ref I'd like the
compiler to enforce that rule.

Comment 35

[Trevor Saunders (:tbsaunde)]

(In reply to Mats Palmgren [:mats] from comment #34)
> > do we always call DestroyFrom() before deleting frames? (I have no idea)
> 
> Yes, it's DestroyFrom() that calls the destructor:
> http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#680
> and "delete aFrame" is forbidden:
> http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#403
> 
> > well, that method's name indicates fairly strongly it mutates stuff
> 
> I don't see why Columns() would suggest that.  We have this naming
> convention in Layout that we only prefix an accessor with Get if it
> may return null, otherwise we skip that.  As far as I know, that
> naming convention applies to all of Gecko (for C++).
> 
> > and not holding a ref when making the call breaks xpcom rules
> 
> Since the caller is required to hold a strong ref I'd like the
> compiler to enforce that rule.

That'd be nice the sticky bit is exactly how do you want it to work exactly?  I don't think we really want to disallow things like
node->GetFirstChild()->Tag() or for this case treeContent->GetColumns()->GetExistingColCount() where your just grabing members and doing readonly side effectless things.  That said if you really think think is this is more risky than the thousands of other places we return raw pointers to things that can be caused to go away I don't care aabout the perf of this that much.

Comment 36

[Trevor Saunders (:tbsaunde)]

Attachment: bug 809871 - nsTreeColumns should store nsTreeBodyFrame* not nsTreeBoxObject*

Comment 37

[Trevor Saunders (:tbsaunde)]

Attachment: bug 809871 - outparamdel some nsTreeBodyFrame methods

Comment 38

[Mats Palmgren (inactive)]

(In reply to Mats Palmgren [:mats] from comment #31)
> > nsTreeColumns::RestoreNaturalOrder()
> 
> Hmm, this method calls SetAttr with 'true' for aNotify.
> Doesn't that mean that the frame, which owns the nsTreeColumns object
> can be destroyed? and if so 'this' can be destroyed when we access
> 'mTree' after the loop, unless the caller holds a strong ref on it...

I found bug 730441 which has a few crash tests.
AFAICT, the suggested changes doesn't seem to make it worse though.

Comment 39

[Mats Palmgren (inactive)]

Comment on attachment 728765 [details] [diff] [review]
bug 809871 - nsTreeColumns should store nsTreeBodyFrame* not nsTreeBoxObject*

r=mats

Comment 40

[Mats Palmgren (inactive)]

Comment on attachment 728766 [details] [diff] [review]
bug 809871 - outparamdel some nsTreeBodyFrame methods

>layout/xul/tree/nsTreeBodyFrame.cpp
>+  int32_t count = std::abs(aCount);

Apparently the C++ Standard std::abs isn't good enough (bug 847480),
so please change std::abs to mozilla::Abs.

>layout/xul/tree/nsTreeBodyFrame.h
>+  already_AddRefed<nsITreeView> GetExistingView() const
>+  {
>+   nsCOMPtr<nsITreeView> view = mView;
>+  return view.forget();
>+  }

indentation

r=mats with those nits fixed

Comment 41

[Trevor Saunders (:tbsaunde)]

(In reply to Mats Palmgren [:mats] from comment #40)
> Comment on attachment 728766 [details] [diff] [review]
> bug 809871 - outparamdel some nsTreeBodyFrame methods
> 
> >layout/xul/tree/nsTreeBodyFrame.cpp
> >+  int32_t count = std::abs(aCount);
> 
> Apparently the C++ Standard std::abs isn't good enough (bug 847480),
> so please change std::abs to mozilla::Abs.

So, I looked at the history here and it looks like my patch is changing it from DeprecatedABS() to std::abs for no reason I can see so I'd guess rebase weirdness.  Do you want it changed to mozilla::abs() or back to mozilla::DeprecatedABS()? the later seems safer since we assign the result to a signed type (although we hardly use)

Comment 42

[Mats Palmgren (inactive)]

mozilla::Abs should be fine here.

Comment 43

[Trevor Saunders (:tbsaunde)]

Attachment: bug 809871 - xul tree accessible creation flushes layout

Comment 44

[alexander :surkov (:asurkov)]

Comment on attachment 728800 [details] [diff] [review]
bug 809871 - xul tree accessible creation flushes layout

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

::: accessible/src/base/nsAccessibilityService.cpp
@@ +1585,3 @@
>      return nullptr;
>  
> +  nsRefPtr<nsTreeColumns> treeCols = treeFrame->Columns();

no null check?

::: accessible/src/xul/XULTreeAccessible.cpp
@@ +44,5 @@
>  {
>    mType = eXULTreeType;
>    mGenericTypes |= eSelect;
>  
> +  nsCOMPtr<nsITreeView> view = aTreeFrame->GetExistingView();

why don't you make it return a raw pointer?

@@ +164,5 @@
> +  NS_ASSERTION(child, "tree without treechildren!");
> +  nsTreeBodyFrame* treeFrame = do_QueryFrame(child->GetPrimaryFrame());
> +  NS_ASSERTION(treeFrame, "xul tree accessible for tree without a frame!");
> +  if (!treeFrame)
> +    return roles::LIST;

null child doesn't make us to return but null treeFrame does, both of them assert, what is the difference?

Comment 45

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #44)
> Comment on attachment 728800 [details] [diff] [review]
> bug 809871 - xul tree accessible creation flushes layout
> 
> Review of attachment 728800 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/src/base/nsAccessibilityService.cpp
> @@ +1585,3 @@
> >      return nullptr;
> >  
> > +  nsRefPtr<nsTreeColumns> treeCols = treeFrame->Columns();
> 
> no null check?

it can't return null so why would there be?

> ::: accessible/src/xul/XULTreeAccessible.cpp
> @@ +44,5 @@
> >  {
> >    mType = eXULTreeType;
> >    mGenericTypes |= eSelect;
> >  
> > +  nsCOMPtr<nsITreeView> view = aTreeFrame->GetExistingView();
> 
> why don't you make it return a raw pointer?

because I suspect mats would prefer it the way it is to prevent people calling methods on the view without holding a ref.

> @@ +164,5 @@
> > +  NS_ASSERTION(child, "tree without treechildren!");
> > +  nsTreeBodyFrame* treeFrame = do_QueryFrame(child->GetPrimaryFrame());
> > +  NS_ASSERTION(treeFrame, "xul tree accessible for tree without a frame!");
> > +  if (!treeFrame)
> > +    return roles::LIST;
> 
> null child doesn't make us to return but null treeFrame does, both of them
> assert, what is the difference?

hm, maybe the assert about the tree frame should go away, I bet its possible for it to reasonably be null during shutdown events.  On the other hand I don't think child can reasonably be null.

Comment 46

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #45)

> > > +  nsRefPtr<nsTreeColumns> treeCols = treeFrame->Columns();
> > 
> > no null check?
> 
> it can't return null so why would there be?

oh, right, then you need to remove the null check from XULTreeAccessible.cpp

> > @@ +164,5 @@
> > > +  NS_ASSERTION(child, "tree without treechildren!");
> > > +  nsTreeBodyFrame* treeFrame = do_QueryFrame(child->GetPrimaryFrame());
> > > +  NS_ASSERTION(treeFrame, "xul tree accessible for tree without a frame!");
> > > +  if (!treeFrame)
> > > +    return roles::LIST;
> > 
> > null child doesn't make us to return but null treeFrame does, both of them
> > assert, what is the difference?
> 
> hm, maybe the assert about the tree frame should go away, I bet its possible
> for it to reasonably be null during shutdown events.  On the other hand I
> don't think child can reasonably be null.

ok, up to you

Comment 47

[Trevor Saunders (:tbsaunde)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/04dfb021fe3f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/eccae9f7fc4b
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/53743e639a87
remote: Trying to insert into pushlog.

Comment 48

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/04dfb021fe3f
https://hg.mozilla.org/mozilla-central/rev/eccae9f7fc4b
https://hg.mozilla.org/mozilla-central/rev/53743e639a87