alexander :surkov (:asurkov) Reporter
	Description • 12 years ago

it's kept addrefed by document cache so we can keep raw pointer. Use http://mxr.mozilla.org/mozilla-central/ to locate nsHTMLLiAccessible.

	Trevor Saunders (:tbsaunde)
	Comment 1 • 12 years ago

(In reply to alexander :surkov from comment #0)
> it's kept addrefed by document cache so we can keep raw pointer. Use
> http://mxr.mozilla.org/mozilla-central/ to locate nsHTMLLiAccessible.

you mean HTMLLiAccessible right?

we should really try and find a way to get rid of that member all together...

	alexander :surkov (:asurkov) Reporter
	Comment 2 • 12 years ago

(In reply to Trevor Saunders (:tbsaunde) from comment #1)
> (In reply to alexander :surkov from comment #0)
> > it's kept addrefed by document cache so we can keep raw pointer. Use
> > http://mxr.mozilla.org/mozilla-central/ to locate nsHTMLLiAccessible.
> 
> you mean HTMLLiAccessible right?

these days yes

> we should really try and find a way to get rid of that member all together...

that's hard to do because we use it for CacheChildren (in case when children were invalidated).

	alexander :surkov (:asurkov) Reporter
	Comment 4 • 12 years ago

Comment on attachment 630273 [details] [diff] [review]
Patch (v1)

redirecting request to Trevor since iirc he had concerns

	Trevor Saunders (:tbsaunde)
	Comment 5 • 12 years ago

> > we should really try and find a way to get rid of that member all together...
> 
> that's hard to do because we use it for CacheChildren (in case when children
> were invalidated).

couldn't we just cheat  by implementing a InvalidateChildren() that doesn't actually uncache it? we'd have to be careful that all callers of InvalidateChildren() either call cacheChildren() next, or don't care about this, which is currently the case faict.

	Trevor Saunders (:tbsaunde)
	Comment 6 • 12 years ago

SO, as it is if you can get the accessible to become defunct and to call  InvalidateChildren()  on it the bullet accessible dies, and mBullet points at freed memory, I'm not absolutely sure that's impossible, though I'd like to think it is.

	Mark Capella [:capella]
	Comment 7 • 12 years ago

I'm sure you and surkov follow this, but I got lost between comment 5 and comment 6 ... 

We're now considering dropping the mBullet member entirely?

You're thinking we create a new virtual InvalidateChildren() for HTMLLIAccessible:: that doesn't uncache the bullet pointer, and that allows us to not worry about AppendChild(mBullet) in CacheChildren()..

I guess at that point HTMLLIAccessible simply defaults to AccessibleWrap::CacheChildren()... 

HTMLLIAccessible::UpdateBullet() stops caring if bullet and accessible are in sync already, and just does its thing.

How is the bullet pointer recognized / referenced in the document cache for a) use in avoiding uncaching, and b) use in HTMLLIAccessible::GetBounds() processing?

	Mark Capella [:capella]
	Comment 8 • 12 years ago

Trev, can you refresh me on this?

	alexander :surkov (:asurkov) Reporter
	Comment 9 • 12 years ago

(In reply to Mark Capella [:capella] from comment #8)
> Trev, can you refresh me on this?

Trevor, ping.

	Trevor Saunders (:tbsaunde)
	Comment 10 • 12 years ago

(In reply to alexander :surkov from comment #9)
> (In reply to Mark Capella [:capella] from comment #8)
> > Trev, can you refresh me on this?
> 
> Trevor, ping.

I'm really not sure what to do here.  It seems to me like the existing patch may introduce a security bug (comment 6), or atleast I'm not completely convinced it won't.

I believe the overriding InvalidateChildren() thing will work, but I'm not sure somebody needs to think about it and investigate.

	alexander :surkov (:asurkov) Reporter
	Comment 11 • 12 years ago

Comment on attachment 630273 [details] [diff] [review]
Patch (v1)

canceling review per comment #10

	diwas joshi [:dij]
	Comment 12 • 10 years ago

hello, is somebody working on this bug because i would like to work on this bug, this would be my first bug so i might need some extra guidance debugging it. thanks!

	Kevin Mather [:kmather73]
	Comment 13 • 9 years ago

Gaurav Pruthi are you still working on this?

	jbonnafo Assignee
	Comment 14 • 8 years ago

Hello,
Is this still considered as a bug to be fixed.
What would be the advantage of having mbullet data member as a raw pointer instead of RefPrt ?
Thanks,

	jbonnafo Assignee
	Comment 15 • 8 years ago

Hello,
How could we move forward on that one ?
Thanks,

	alexander :surkov (:asurkov) Reporter
	Comment 16 • 7 years ago

we don't have InvalidateChildren anymore, and it should be safe to switch to a raw pointer. One issue we should address though, UpdateBullet() should call UnbindFromDocument() to destroy existing bullet.

	jbonnafo Assignee
	Comment 17 • 7 years ago

Hello,

If i have well understood the last comment, a new patch proposal needs to be prepared in to have:
UpdateBullet() calling UnbindFromDocument() to destroy existing bullet.

I would be happy to try to help on this one.

Thanks,

	alexander :surkov (:asurkov) Reporter
	Comment 18 • 7 years ago

thanks! changing assignee per comment #17

	jbonnafo Assignee
	Comment 19 • 7 years ago

Turned nsHTMLLiAccessible::mBullet into raw pointer and UpdateBullet() calling UnbindFromDocument() to destroy existing bullet.

	alexander :surkov (:asurkov) Reporter
	Comment 20 • 7 years ago

Comment on attachment 8822855 [details] [diff] [review]
Turn nsHTMLLiAccessible::mBullet into raw pointer and destroy it accordingly

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

::: accessible/html/HTMLListAccessible.cpp
@@ +116,5 @@
>  
>    TreeMutation mt(this);
>    if (aHasBullet) {
> +    // Destroy current HTMLListBulletAccessible object
> +    mDoc->UnbindFromDocument(mBullet);

at second glance UnbindFromDocument call is not needed, because TreeMutation takes care to fire to hide event in case of !aHasBullet, which will call ShutdownChildrenInSubtree.

Please remove these lines, r=me with that.

	jbonnafo Assignee
	Comment 21 • 7 years ago

Hello,
i have amended patch proposal accordingly.
Thanks,

	Ryan VanderMeulen [:RyanVM]
	Comment 22 • 7 years ago

checkin=needed is for when the patch has proper reviews and is ready to land.

	alexander :surkov (:asurkov) Reporter
	Comment 23 • 7 years ago

Comment on attachment 8823124 [details] [diff] [review]
Bug 752823 - turn nsHTMLLiAccessible::mBullet into raw pointer, r:surkov

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

eventually I forgot to r+ it

	Pulsebot
	Comment 24 • 7 years ago

Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd6d4ca20668
Turn nsHTMLLiAccessible::mBullet into raw pointer. r=surkov

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 25 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/dd6d4ca20668