Closed
Bug 752823
Opened 13 years ago
Closed 8 years ago
turn nsHTMLLiAccessible::mBullet into raw pointer
Categories
(Core :: Disability Access APIs, defect)
Core
Disability Access APIs
Tracking
()
RESOLVED
FIXED
mozilla53
Tracking | Status | |
---|---|---|
firefox53 | --- | fixed |
People
(Reporter: surkov, Assigned: jeanluc.bonnafoux, Mentored)
References
(Blocks 1 open bug)
Details
(Whiteboard: [good first bug][lang=c++])
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
surkov
:
review+
|
Details | Diff | Splinter Review |
it's kept addrefed by document cache so we can keep raw pointer. Use http://mxr.mozilla.org/mozilla-central/ to locate nsHTMLLiAccessible.
Comment 1•13 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...
Reporter | ||
Comment 2•13 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).
Comment 3•13 years ago
|
||
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
Attachment #630273 -
Flags: review?(surkov.alexander)
Reporter | ||
Comment 4•13 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)
Comment 5•13 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.
Comment 6•13 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.
Comment 7•13 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?
Comment 8•12 years ago
|
||
Trev, can you refresh me on this?
Updated•12 years ago
|
Assignee: markcapella → nobody
Status: ASSIGNED → NEW
Reporter | ||
Comment 9•12 years ago
|
||
(In reply to Mark Capella [:capella] from comment #8)
> Trev, can you refresh me on this?
Trevor, ping.
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.
Reporter | ||
Comment 11•12 years ago
|
||
Attachment #630273 -
Flags: review?(trev.saunders)
Updated•10 years ago
|
Mentor: trev.saunders
Whiteboard: [good first bug][mentor=trev.saunders@gmail.com][lang=c++] → [good first bug][lang=c++]
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!
Updated•10 years ago
|
Assignee: nobody → gaurav.pruthi88
Comment 13•9 years ago
|
||
Gaurav Pruthi are you still working on this?
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,
Assignee | ||
Comment 15•8 years ago
|
||
Hello,
How could we move forward on that one ?
Thanks,
Updated•8 years ago
|
Flags: needinfo?(surkov.alexander)
Reporter | ||
Comment 16•8 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.
Flags: needinfo?(surkov.alexander)
Assignee | ||
Comment 17•8 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,
Reporter | ||
Comment 18•8 years ago
|
||
thanks! changing assignee per comment #17
Assignee: gaurav.pruthi88 → jeanluc.bonnafoux
Assignee | ||
Comment 19•8 years ago
|
||
Turned nsHTMLLiAccessible::mBullet into raw pointer and UpdateBullet() calling UnbindFromDocument() to destroy existing bullet.
Attachment #8822855 -
Flags: review?(surkov.alexander)
Reporter | ||
Comment 20•8 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.
Attachment #8822855 -
Flags: review?(surkov.alexander) → review+
Assignee | ||
Comment 21•8 years ago
|
||
Hello,
i have amended patch proposal accordingly.
Thanks,
Attachment #8822855 -
Attachment is obsolete: true
Attachment #8823124 -
Flags: review?(surkov.alexander)
Keywords: checkin-needed
Updated•8 years ago
|
Attachment #630273 -
Attachment is obsolete: true
Comment 22•8 years ago
|
||
checkin=needed is for when the patch has proper reviews and is ready to land.
Keywords: checkin-needed
Reporter | ||
Comment 23•8 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
Attachment #8823124 -
Flags: review?(surkov.alexander) → review+
Keywords: checkin-needed
Comment 24•8 years 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•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years 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.
Description
•