Investigate and fix unsafe selection changes

RESOLVED FIXED

Status

()

RESOLVED FIXED
9 years ago
3 years ago

People

(Reporter: davidb, Assigned: davidb)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?][critsmash:investigating])

Attachments

(2 attachments, 1 obsolete attachment)

davidb, who owns this?
Whiteboard: [sg:critical?] → [sg:critical?][critsmash:investigating]
(In reply to comment #1)
> davidb, who owns this?

Sorry. Me. I have a plan - patch expected by end of Monday.
Assignee: nobody → bolterbugz
Created attachment 450563 [details] [diff] [review]
WIP

Might be this simple.
Attachment #450563 - Attachment is obsolete: true
nit: double space: nsDocAccessible  *docAccessible = 

you don't want them to coalesce?

mochitest please
(In reply to comment #5)
> nit: double space: nsDocAccessible  *docAccessible = 
> 
> you don't want them to coalesce?
> 

No I want to preserve behaviour, at least for now.

> mochitest please

Yep.
Created attachment 451056 [details] [diff] [review]
test

Sorry for delay. Here's a test.
Attachment #451056 - Flags: review?
Attachment #451056 - Flags: review? → review?(marco.zehe)

Updated

9 years ago
Attachment #451056 - Flags: review?(marco.zehe) → review+

Comment 9

9 years ago
Comment on attachment 451056 [details] [diff] [review]
test

r=me, thanks!
Attachment #450564 - Flags: review?(surkov.alexander)
Comment on attachment 450564 [details] [diff] [review]
WIP

(In reply to comment #5)
> nit: double space: nsDocAccessible  *docAccessible = 

fixed locally.

> you don't want them to coalesce?

No (as per IRC)

> mochitest please

Added (see other attached diff). We can do more of course, but let's do that on a follow up (Bug 571900)?
Comment on attachment 451056 [details] [diff] [review]
test

>+    function addSelection(aNode, aOption)
>+    {

>+      this.getID = function addselection_getID() {
>+        return prettyName(this.DOMNode) + " added to selection";

Note to self: change this to this.optionNode
Comment on attachment 450564 [details] [diff] [review]
WIP


>+  nsDocAccessible  *docAccessible = 

nit: two spaces

>+    nsAccUtils::GetDocAccessibleFor(optionNode);

use GetDocAccessible();

>+  if (!docAccessible)
>+    return;

we don't need a check while IsDefuct() is called 

>+
>+  nsRefPtr<nsAccEvent> eventSelectionWithin =
>+    new nsAccEvent(nsIAccessibleEvent::EVENT_SELECTION_WITHIN, multiSelect);

selectionWithinEvent or selWithinEvent?

>+  if (eventSelectionWithin)

fail if no memory

>-  nsEventShell::FireEvent(eventType, optionAccessible);
>+  nsRefPtr<nsAccEvent> eventAddRemove =

addRemoveSelectionEvent or addRemoveSelEvent?
Attachment #450564 - Flags: review?(surkov.alexander) → review+
(In reply to comment #12)
> (From update of attachment 450564 [details] [diff] [review])

OK everything updated except:

> >+  nsRefPtr<nsAccEvent> eventSelectionWithin =
> 
> selectionWithinEvent or selWithinEvent?
> 
> >+  nsRefPtr<nsAccEvent> eventAddRemove =
> 
> addRemoveSelectionEvent or addRemoveSelEvent?

I think it is clearer to leave these. Otherwise the meaning is confusing.
we just used reorderEvent or refreshEvent for example, should be they converted to eventReorder and etc?

btw, eventAddRemove doesn't say it's about selection. If it is then eventWithin should be enough as well.
OK I made the selFooEvent.
Test landed: http://hg.mozilla.org/mozilla-central/rev/cfdfc56b8e73
Fix landed: http://hg.mozilla.org/mozilla-central/rev/d2c022e9417f
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Updated

3 years ago
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.