turn nsHTMLLiAccessible::mBullet into raw pointer

RESOLVED FIXED in Firefox 53

Status

()

Core
Disability Access APIs
RESOLVED FIXED
5 years ago
4 months ago

People

(Reporter: surkov, Assigned: jbonnafo, Mentored)

Tracking

(Blocks: 1 bug)

unspecified
mozilla53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

(Whiteboard: [good first bug][lang=c++])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 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.
(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...
(Reporter)

Comment 2

5 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).
Created attachment 630273 [details] [diff] [review]
Patch (v1)
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
Attachment #630273 - Flags: review?(surkov.alexander)
(Reporter)

Comment 4

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

redirecting request to Trevor since iirc he had concerns
Attachment #630273 - Flags: review?(surkov.alexander) → review?(trev.saunders)
> > 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.
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.
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?
Trev, can you refresh me on this?
Assignee: markcapella → nobody
Status: ASSIGNED → NEW
(Reporter)

Comment 9

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

Trevor, ping.
(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.
(Reporter)

Comment 11

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

canceling review per comment #10
Attachment #630273 - Flags: review?(trev.saunders)
Mentor: trev.saunders@gmail.com
Whiteboard: [good first bug][mentor=trev.saunders@gmail.com][lang=c++] → [good first bug][lang=c++]

Comment 12

3 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!

Updated

2 years ago
Assignee: nobody → gaurav.pruthi88
Gaurav Pruthi are you still working on this?
(Assignee)

Comment 14

5 months 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,
(Assignee)

Comment 15

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

Updated

5 months ago
Flags: needinfo?(surkov.alexander)
(Reporter)

Comment 16

4 months 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.
Flags: needinfo?(surkov.alexander)
(Assignee)

Comment 17

4 months 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,
(Reporter)

Comment 18

4 months ago
thanks! changing assignee per comment #17
Assignee: gaurav.pruthi88 → jeanluc.bonnafoux
(Assignee)

Updated

4 months ago
Status: NEW → ASSIGNED
(Assignee)

Comment 19

4 months ago
Created attachment 8822855 [details] [diff] [review]
Turn nsHTMLLiAccessible::mBullet into raw pointer and destroy it accordingly

Turned nsHTMLLiAccessible::mBullet into raw pointer and UpdateBullet() calling UnbindFromDocument() to destroy existing bullet.
Attachment #8822855 - Flags: review?(surkov.alexander)
(Reporter)

Comment 20

4 months 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.
Attachment #8822855 - Flags: review?(surkov.alexander) → review+
(Assignee)

Comment 21

4 months ago
Created attachment 8823124 [details] [diff] [review]
Bug 752823 - turn nsHTMLLiAccessible::mBullet into raw pointer, r:surkov

Hello,
i have amended patch proposal accordingly.
Thanks,
Attachment #8822855 - Attachment is obsolete: true
Attachment #8823124 - Flags: review?(surkov.alexander)
(Assignee)

Updated

4 months ago
Keywords: checkin-needed
Attachment #630273 - Attachment is obsolete: true
checkin=needed is for when the patch has proper reviews and is ready to land.
Keywords: checkin-needed
(Reporter)

Comment 23

4 months 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
Attachment #8823124 - Flags: review?(surkov.alexander) → review+
(Assignee)

Updated

4 months ago
Keywords: checkin-needed

Comment 24

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

Comment 25

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/dd6d4ca20668
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.