Closed Bug 768243 Opened 12 years ago Closed 12 years ago

Assertion failure: i < Length() (invalid array index), at ../../../dist/include/nsTArray.h:535

Categories

(Core :: Disability Access APIs, defect)

Product:
Core
Component:
Disability Access APIs
Platform:
ARM
Android
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla20
Tracking Flags:
Tracking Status
firefox16 --- wontfix
firefox17 - wontfix
firefox18 + fixed
firefox19 --- fixed
firefox20 --- fixed
firefox-esr10 18+ fixed
firefox-esr17 18+ fixed
fennec + ---

People

(Reporter: decoder, Assigned: tbsaunde)

References

(Blocks 1 open bug)

Details

(Keywords: sec-critical, testcase, Whiteboard: [adv-main18+][adv-esr17+][adv-esr10+])

Attachments

(5 files, 3 obsolete files)

Test case for browser
3.64 KB, application/xhtml+xml
Details
backtraces from GDB
17.65 KB, text/plain
Details
clearer test case
1.98 KB, text/html
Details
patch v3
1.66 KB, patch
Details | Diff | Splinter Review
patch: what I kept in mind
1.52 KB, patch
tbsaunde
: review+
akeybl
: approval-mozilla-aurora+
akeybl
: approval-mozilla-beta+
akeybl
: approval-mozilla-release-
akeybl
: approval-mozilla-esr10+
akeybl
: approval-mozilla-esr17+
Details | Diff | Splinter Review
Attached file Test case for browser β€” 
The attached HTML code triggers an assertion on Fennec Native (mozilla-central, debug build, rev e240d6e43c9a, might need 2-3 runs to actually abort).

The assertion did not trigger on a Firefox desktop debug build for me. Marking s-s because this could be an out-of-range problem.
Do you have a stack for the assertion?
minidump_stackwalk isn't showing me any symbols even using the crashreporter symbols obtained by "make buildsymbols". Any idea what I might be doing wrong? I also have an own implementation that uses addr2line on the minidump_stackwalk addresses, but it looks pretty broken quite often with Fennec stuff:

#0      mozilla::widget::GfxInfoBase::Observe(nsISupports*, char const*, unsigned short const*) at libgcc2.c:0
#1      $d at nsDesktopNotification.cpp:0
#2      mozilla::widget::GfxInfoBase::Observe(nsISupports*, char const*, unsigned short const*) at libgcc2.c:0
#3      std::priv::__write_float(std::priv::__basic_iostring<char>&, int, int, double) at /srv/repos/android-ndk-r6b/sources/cxx-stl/stlport/src/num_put_float.cpp:823
#4      $d at nsDesktopNotification.cpp:0
#5      $d at nsDesktopNotification.cpp:0
#6      $t at nsBaseFilePicker.cpp:0
#7      std::priv::__write_float(std::priv::__basic_iostring<char>&, int, int, double) at /srv/repos/android-ndk-r6b/sources/cxx-stl/stlport/src/num_put_float.cpp:823
[...]

I can try other stuff if you have any suggestions :)
The best I can suggest is running the app under gdb :)
(In reply to Ehsan Akhgari [:ehsan] from comment #3)
> The best I can suggest is running the app under gdb :)

How would I do that on mobile?
(In reply to Christian Holler (:decoder) from comment #4)
> (In reply to Ehsan Akhgari [:ehsan] from comment #3)
> > The best I can suggest is running the app under gdb :)
> 
> How would I do that on mobile?

https://wiki.mozilla.org/Mobile/Fennec/Android#Debugging
(In reply to Ehsan Akhgari [:ehsan] from comment #5)
> 
> https://wiki.mozilla.org/Mobile/Fennec/Android#Debugging

Doesn't work for me, the gdbserver build fails with missing termcap on my host. I've tried various things but wasn't able to resolve it. Someone with a working setup should try the test here with a debug build.
You can also jump on #mobile and ask for help from the folks there.
The "i < Length()" in TArray is generally a non-trivial problem. How can we get more info here.
Keywords: sec-critical, testcase
tracking-fennec: --- → ?
Mark, Brad, any idea how/why nsDesktopNotification is involved in this stack on Fennec? (see comment 2)
Adding Ehsan as assignee, Ehsan feel free to bump over to someone who can get you the gdb stack if necessary.
Assignee: nobody → ehsan
David, would you mind finding someone else to investigate this?  I'm buried under other work :(
Assignee: ehsan → nobody
Mats can you help find someone to work on this one?
Assignee: nobody → matspal
I don't see anything in this bug that would suggest it's a Layout bug.
Also, I don't have a rooted phone and I haven't figured out how to run
'gdbserve' on it, so I can't get a stack :-(

Can someone from the mobile team dig up the correct stack so we can
assign it to the right team?
Assignee: matspal → mark.finkle
Assignee: mark.finkle → chrislord.net
tracking-fennec: ? → 16+
Hi Chris, any traction on this bug? Can you recreate it?
(In reply to David Bolter [:davidb] from comment #14)
> Hi Chris, any traction on this bug? Can you recreate it?

Sorry, I'm at a conference at the moment, then on a week's holiday - I'm doing a build now to see if I can reproduce and at least attach a backtrace (and a fix if it's trivial), but it may be sensible to find someone else to work on this.

I may not have regular internet access after Monday, so if I make no headway before then, I'll unassign this bug.
(In reply to Christian Holler (:decoder) from comment #0)
> Created attachment 636508 [details]
> Test case for browser
> 
> The attached HTML code triggers an assertion on Fennec Native
> (mozilla-central, debug build, rev e240d6e43c9a, might need 2-3 runs to
> actually abort).
> 
> The assertion did not trigger on a Firefox desktop debug build for me.
> Marking s-s because this could be an out-of-range problem.

Can you give us some details about the device you tested on? Building mozilla-central (rev 8b96a33ecbd2), I can't immediately replicate this on a Galaxy Nexus.

Not finding much time to look at this what with other things going on here - I'll continue to dive in the time I get, but it'd be sensible to reassign this.
(In reply to Chris Lord [:cwiiis] from comment #16)

> Can you give us some details about the device you tested on? Building
> mozilla-central (rev 8b96a33ecbd2), I can't immediately replicate this on a
> Galaxy Nexus.

This was found on one of the Tegra boards we use for fuzzing. I assume your build is a debug build too?
Ok, am able to reproduce this, didn't realise I didn't have a debug build. Unfortunately, the stack is always corrupt whenever it aborts for me.

Before the crash, I do get this other assertion:

###!!! ASSERTION: Adopting child!: 'Error', file /home/cwiiis/Projects/mozilla-central/accessible/src/generic/Accessible.cpp, line 2471

Whether that gives any hints as to the cause though, I don't know.

I'm not going to have time to look at this before I go on holiday, so I'm unassigning. Feel free to reassign back to me if no one's looked at it in the next week.
Assignee: chrislord.net → nobody
I still crash with the pref accessibility.forced_disabled=1 so the
a11y assertion is likely just a symptom of an earlier error.

Chris, do you get a different result with that pref set?
I still crash with the pref accessibility.forced_disabled=1 so the
a11y assertion is likely just a symptom of an earlier error.

Chris, do you get a different result with that pref set?
Correction, the pref name is accessibility.force_disabled
Assignee: nobody → matspal
why is this sec-critical?
tracking-fennec: 16+ → +
(In reply to Brad Lassey [:blassey] from comment #22)
> why is this sec-critical?

I think it may be educated-speculative. Unfortunately Dan is attempting PTO until Sept 4.
> why is this sec-critical?

The "i < Length() (invalid array index)" hints at a possible buffer overflow,
so until we actually know what the crash is this bug should stay hidden.
Per comment 13, I don't know how to make progress here without a stack.
Assignee: matspal → nobody
Can we get a mobile stack for the test case?
Kevin can you get us a stack?
Assignee: nobody → kbrosnan
Here is a stack. The index being requested is 1, so that seems sane.

0x508f7730 in ElementAt (i=<optimized out>, this=<optimized out>) at ../../../dist/include/nsTArray.h:535
535	    MOZ_ASSERT(i < Length(), "invalid array index");
(gdb) bt
#0  0x508f7730 in ElementAt (i=<optimized out>, this=<optimized out>) at ../../../dist/include/nsTArray.h:535
#1  ContentChildAt (this=<optimized out>, aIndex=<optimized out>) at /home/kats/zspace/mozilla-git/accessible/src/generic/Accessible.h:416
#2  Accessible::ContentChildAt (this=<optimized out>, aIndex=<optimized out>) at /home/kats/zspace/mozilla-git/accessible/src/generic/Accessible.h:415
#3  0x508f7c32 in DocAccessible::CacheChildrenInSubtree (this=0x51f82cd0, aRoot=0x52214b20) at /home/kats/zspace/mozilla-git/accessible/src/generic/DocAccessible.cpp:2002
#4  0x508fa45a in DocAccessible::UpdateTreeInternal (this=0x51f82cd0, aChild=0x52214b20, aIsInsert=<optimized out>) at /home/kats/zspace/mozilla-git/accessible/src/generic/DocAccessible.cpp:1922
#5  0x508fa60c in DocAccessible::UpdateTree (this=0x51f82cd0, aContainer=0x51f82cd0, aChildNode=0x51f41b50, aIsInsert=<optimized out>)
    at /home/kats/zspace/mozilla-git/accessible/src/generic/DocAccessible.cpp:1868
#6  0x508fa794 in DocAccessible::ProcessContentInserted (this=0x51f82cd0, aContainer=<optimized out>, aInsertedContent=0x4dfba654) at /home/kats/zspace/mozilla-git/accessible/src/generic/DocAccessible.cpp:1840
#7  0x508de0b0 in NotificationController::ContentInsertion::Process (this=0x4dfba640) at /home/kats/zspace/mozilla-git/accessible/src/base/NotificationController.cpp:838
#8  0x508de904 in NotificationController::WillRefresh (this=0x4d47e560, aTime=...) at /home/kats/zspace/mozilla-git/accessible/src/base/NotificationController.cpp:230
#9  0x50087992 in nsRefreshDriver::Notify (this=0x4c6a1100, aTimer=<optimized out>) at /home/kats/zspace/mozilla-git/layout/base/nsRefreshDriver.cpp:339
#10 0x50a91b8a in nsTimerImpl::Fire (this=0x524c1b00) at /home/kats/zspace/mozilla-git/xpcom/threads/nsTimerImpl.cpp:476
#11 0x50a91d38 in nsTimerEvent::Run (this=0x4dd89080) at /home/kats/zspace/mozilla-git/xpcom/threads/nsTimerImpl.cpp:556
#12 0x50a8ddea in nsThread::ProcessNextEvent (this=0x4dd510f0, mayWait=<optimized out>, result=0x4ab8a897) at /home/kats/zspace/mozilla-git/xpcom/threads/nsThread.cpp:624
#13 0x50a5a24e in NS_ProcessNextEvent_P (thread=0x4dd510f0, mayWait=<optimized out>) at /home/kats/zspace/mozilla-git/obj-android-debug/xpcom/build/nsThreadUtils.cpp:220
#14 0x5096dfe2 in mozilla::ipc::MessagePump::Run (this=0x4dd532e0, aDelegate=0x4dd7f0e0) at /home/kats/zspace/mozilla-git/ipc/glue/MessagePump.cpp:82
#15 0x50abb29a in MessageLoop::RunInternal (this=0x4dd7f0e0) at /home/kats/zspace/mozilla-git/ipc/chromium/src/base/message_loop.cc:208
#16 0x50abb2f8 in RunHandler (this=0x4dd7f0e0) at /home/kats/zspace/mozilla-git/ipc/chromium/src/base/message_loop.cc:201
#17 MessageLoop::Run (this=0x4dd7f0e0) at /home/kats/zspace/mozilla-git/ipc/chromium/src/base/message_loop.cc:175
#18 0x508c1ade in nsBaseAppShell::Run (this=0x4dd62660) at /home/kats/zspace/mozilla-git/widget/xpwidgets/nsBaseAppShell.cpp:163
#19 0x50790578 in nsAppStartup::Run (this=0x4c6fc550) at /home/kats/zspace/mozilla-git/toolkit/components/startup/nsAppStartup.cpp:273
#20 0x4fedeeea in XREMain::XRE_mainRun (this=0x4ab8aac4) at /home/kats/zspace/mozilla-git/toolkit/xre/nsAppRunner.cpp:3800
#21 0x4fee0606 in XREMain::XRE_main (this=0x4ab8aac4, argc=<optimized out>, argv=0x4dd6b048, aAppData=<optimized out>) at /home/kats/zspace/mozilla-git/toolkit/xre/nsAppRunner.cpp:3877
#22 0x4fee0762 in XRE_main (argc=7, argv=0x4dd6b048, aAppData=0x4a5a7c1c, aFlags=<optimized out>) at /home/kats/zspace/mozilla-git/toolkit/xre/nsAppRunner.cpp:3953
#23 0x4fee61e0 in GeckoStart (data=0x2a0debd8, appData=0x4a5a7c1c) at /home/kats/zspace/mozilla-git/toolkit/xre/nsAndroidStartup.cpp:73
#24 0x4a58e028 in Java_org_mozilla_gecko_GeckoAppShell_nativeRun (jenv=0x2a0330d8, jc=<optimized out>, jargs=0x20400005) at /home/kats/zspace/mozilla-git/mozglue/android/APKOpen.cpp:983
#25 0x4070fe34 in dvmPlatformInvoke () from /home/kats/android/jdb/moz-gdb/lib/emulator-5554/system/lib/libdvm.so
#26 0x4073ee6a in dvmCallJNIMethod(unsigned int const*, JValue*, Method const*, Thread*) () from /home/kats/android/jdb/moz-gdb/lib/emulator-5554/system/lib/libdvm.so
#27 0x4072ad88 in dvmCheckCallJNIMethod(unsigned int const*, JValue*, Method const*, Thread*) () from /home/kats/android/jdb/moz-gdb/lib/emulator-5554/system/lib/libdvm.so
#28 0x40740f84 in dvmResolveNativeMethod(unsigned int const*, JValue*, Method const*, Thread*) () from /home/kats/android/jdb/moz-gdb/lib/emulator-5554/system/lib/libdvm.so
#29 0x40719264 in dvmJitToInterpNoChain () from /home/kats/android/jdb/moz-gdb/lib/emulator-5554/system/lib/libdvm.so
#30 0x40719264 in dvmJitToInterpNoChain () from /home/kats/android/jdb/moz-gdb/lib/emulator-5554/system/lib/libdvm.so
Also, FWIW I also get the assertion error from comment #18, twice, but now pointing at line 2457 (presumably because I'm using a more recent m-c).
Thanks Kats. I'm hoping this stack illuminates Mats.
Assignee: kbrosnan → matspal
That stack seems to implicate accessibility code but I alse crashed
with accessibility.force_disabled=1 so it doesn't seem like the real
culprit.  What's the stack with that pref set?  Also, what's the stack
for the "Adopting child!" assertion?
Attached file backtraces from GDB β€” 
Attached are the backtraces for the NS_ERROR as well as the backtrace for the crash. When I set accessibility.force_disabled=1 I still get the exact same behaviour and stacks. It's like that pref doesn't affect anything.
Hmm, that's odd.  Maybe it's not possible to disable a11y on Android?

Since the crash is in a11y code, let's try there first.
Assignee: matspal → nobody
Component: Layout → Disability Access APIs
CC+ Hub for the pref, and Eitan for mobile a11y.
The pref to disable a11y is platform specific. It is only on Mac and Windows at the moment.
why don't we just build with the --disable-accessibility confiure flag and see what happens?

btw I haven't looked at the test yet, but I'm a little suprised a11y is on in the first place.
Assignee: nobody → eitan
I stand corrected, it is also on Atk. But not on Android.
I am pretty sure I see the issue in AccessFu, headed to a flight in 3 minutes. I'll do it on the other side!
I haven't been able to reproduce this on x86_64 debug builds, or unoptimized arm debug builds in the hope its timing dependant I'm trying a arm optimized debug build.

Surkov happen to have any ideas?  I suspect roughly what is ahppening is that a kid is somehow getting removed during the children chacing stuff probably in CacheChildren() called by EnsureChildren() but that's just a guess.
(In reply to Trevor Saunders (:tbsaunde) from comment #39)

> Surkov happen to have any ideas?  I suspect roughly what is ahppening is
> that a kid is somehow getting removed during the children chacing stuff
> probably in CacheChildren() called by EnsureChildren() but that's just a
> guess.

somebody stolen a child (per comment #18). not sure how it could happen during recursive DocAccessible::CacheChildrenInSubtree. Somebody who hits this assertion should start investigation from that point.
Eitan mitigated this on another bug but the core problem remains and still affects users of accessibility services on android. Trevor agreed to take this one.
Assignee: eitan → trev.saunders
Having trouble with traction here.

Christian, can you still recreate this bug?
(In reply to David Bolter [:davidb] from comment #42)
> Having trouble with traction here.
> 
> Christian, can you still recreate this bug?

The core issue still exists.  However I don't think there is a quick fix basically InvalidateChildren() can detach whole subtrees of accessibles, then a node for a non root accessible in that subtree can get reparented, and we end up adopting accessible changing its parent which breaks things.  I'm not convinced this is actually exploitable though, I don't believe we'll remove the reparented accessible from the doc accessible cache while in CacheChildrenInSubtree() where the assert is so while we do access a out of bounds element of the array that element should I believe still always point at an object that is actually still alive.  That's not to say this isn't a bad bug, or that there aren't big issues with a11y tree update.
Hardware: All → ARM
Are you saying that the nsTArray is a subset of a bigger array guaranteed to have a longer lifetime?
(In reply to Daniel Veditz [:dveditz] from comment #44)
> Are you saying that the nsTArray is a subset of a bigger array guaranteed to
> have a longer lifetime?

Not exactly, what I'm saying is that I believe to end up in this situation I believe an entry must have been removed from one array and added to another.  So I guess this is actually exploitable if you can arrange for that removal to cause the array's buffer to be reallocated at a smaller size.  I'm fairly confident this bug has been around for a long long time.

In any case I think I'm getting close to a patch, but it'll be a bit scary to take on branches, because it changes how the tree of accessible objects is updated in significant and fundimental ways.
Attached patch wip (obsolete) β€” β€” Splinter Review 
I need to work on some of the odd ball CacheChildren() over rides, but this fixes the assert for me.

This passes all tests except treeupdate/test_select.html where a reorder event is never fired.
Comment on attachment 675562 [details] [diff] [review]
wip

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

You didn't catch a cause of children caching reentrancing problem?

::: accessible/src/generic/DocAccessible.cpp
@@ +1524,5 @@
> +    if (childIdx >= ContentChildCount() || child != ContentChildAt(childIdx))
> +      InsertChildAt(childIdx, child);
> +
> +    childIdx++;
> +  }

some comment to the patch is wanted, why does AppendChild() doesn't work?

@@ +1844,5 @@
>    // children because there's no good way to find insertion point of new child
>    // accessibles into accessible tree. We need to invalidate children even
>    // there's no inserted accessibles in the end because accessible children
>    // are created while parent recaches child accessibles.
> +  aContainer->CacheChildren();

this can lead to lost accessible subtrees
> You didn't catch a cause of children caching reentrancing problem?

what? I don't know what problem your talking about.

> ::: accessible/src/generic/DocAccessible.cpp
> @@ +1524,5 @@
> > +    if (childIdx >= ContentChildCount() || child != ContentChildAt(childIdx))
> > +      InsertChildAt(childIdx, child);
> > +
> > +    childIdx++;
> > +  }
> 
> some comment to the patch is wanted, why does AppendChild() doesn't work?

because the accessible may already have children some of which could be for child accessibles that come after this one.

> @@ +1844,5 @@
> >    // children because there's no good way to find insertion point of new child
> >    // accessibles into accessible tree. We need to invalidate children even
> >    // there's no inserted accessibles in the end because accessible children
> >    // are created while parent recaches child accessibles.
> > +  aContainer->CacheChildren();
> 
> this can lead to lost accessible subtrees

I'm not sure I understand what you mean here
(In reply to Trevor Saunders (:tbsaunde) from comment #48)
> > You didn't catch a cause of children caching reentrancing problem?
> 
> what? I don't know what problem your talking about.

I thought that was a problem you said on irc one day but I might be wrong. Anyway, I was interesting to know a root of the problem. I think you stated it in comment #43. So did you manage to understand how "InvalidateChildren() can detach whole subtrees of accessibles, then a node for a non root accessible in that subtree can get reparented, and we end up adopting accessible" happens?

> > ::: accessible/src/generic/DocAccessible.cpp
> > @@ +1524,5 @@
> > > +    if (childIdx >= ContentChildCount() || child != ContentChildAt(childIdx))
> > > +      InsertChildAt(childIdx, child);
> > > +
> > > +    childIdx++;
> > > +  }
> > 
> > some comment to the patch is wanted, why does AppendChild() doesn't work?
> 
> because the accessible may already have children some of which could be for
> child accessibles that come after this one.

is it because of UpdateChildren() on CacheChildren() replacement in your patch or is it issue in existing code? If second then it's interesting also to know how it happens.

> > @@ +1844,5 @@
> > >    // children because there's no good way to find insertion point of new child
> > >    // accessibles into accessible tree. We need to invalidate children even
> > >    // there's no inserted accessibles in the end because accessible children
> > >    // are created while parent recaches child accessibles.
> > > +  aContainer->CacheChildren();
> > 
> > this can lead to lost accessible subtrees
> 
> I'm not sure I understand what you mean here

I don't really recall details, I'm not sure but iirc it was caused by missed notifications from layout and thus if we don't invalidateChildren then we miss some accessible (and thus its tree). On irc you said you miss reorder event, it might be a case.
(In reply to alexander :surkov from comment #49)
> (In reply to Trevor Saunders (:tbsaunde) from comment #48)
> > > You didn't catch a cause of children caching reentrancing problem?
> > 
> > what? I don't know what problem your talking about.
> 
> I thought that was a problem you said on irc one day but I might be wrong.
> Anyway, I was interesting to know a root of the problem. I think you stated
> it in comment #43. So did you manage to understand how "InvalidateChildren()
> can detach whole subtrees of accessibles, then a node for a non root
> accessible in that subtree can get reparented, and we end up adopting
> accessible" happens?

yes, I believe that's the problem.  I didn't really look into how it happens to much since it seems like detaching subtrees is flaw in the design of InvalidateChildren(), which as far as I can tell can easily do that.

> > > ::: accessible/src/generic/DocAccessible.cpp
> > > @@ +1524,5 @@
> > > > +    if (childIdx >= ContentChildCount() || child != ContentChildAt(childIdx))
> > > > +      InsertChildAt(childIdx, child);
> > > > +
> > > > +    childIdx++;
> > > > +  }
> > > 
> > > some comment to the patch is wanted, why does AppendChild() doesn't work?
> > 
> > because the accessible may already have children some of which could be for
> > child accessibles that come after this one.
> 
> is it because of UpdateChildren() on CacheChildren() replacement in your
> patch or is it issue in existing code? If second then it's interesting also
> to know how it happens.

its a result of the change from UpdateChildren() to CacheChildren(0.

The idea of the change is basically that instead of blowing away all the kids, and then looking to see what all kids should get added you'll just go through and see what new kids exist and put them were they belong.

> > > @@ +1844,5 @@
> > > >    // children because there's no good way to find insertion point of new child
> > > >    // accessibles into accessible tree. We need to invalidate children even
> > > >    // there's no inserted accessibles in the end because accessible children
> > > >    // are created while parent recaches child accessibles.
> > > > +  aContainer->CacheChildren();
> > > 
> > > this can lead to lost accessible subtrees
> > 
> > I'm not sure I understand what you mean here
> 
> I don't really recall details, I'm not sure but iirc it was caused by missed
> notifications from layout and thus if we don't invalidateChildren then we
> miss some accessible (and thus its tree). On irc you said you miss reorder
> event, it might be a case.

I'm not sure what it was that was caused.

If there's a case layout doesn't notify us of removal that could probably help to create the situation in the first place which you wonder about above.  If there is such a case layout should be changed to notify us instead of relying on InvalidateChildren().

That may be the cause but I believe this patch may currently cause us to miss show events in some cases when we find kids, but I'm not really clear on the stuff in UpdateTree() is supposed to work to fire events when other things update the tree.
Comment on attachment 675562 [details] [diff] [review]
wip

saddly this doesn't fix the issue I was just missing the asserts somehow
Attachment #675562 - Attachment is obsolete: true
Attached file clearer test case β€” 
I took the original test case, removed a bunch of garbage that was clearly unneeded, and renamed things to hopefully be clearer.  I get asserts with this test case too to be clear.
Boris, For some reason in this test case we miss some notifications that content was removed.  Which causes us to not remove accessibles for that removed content from the tree.  THen when that content is reinserted there are still accessibles around for the content where it was before.  Would you have ideas why layout doesn't call nsAccessibilityService::ContentRemoved()? or would you have suggestions on where to debug?
Trevor, which DOM node did you not notifications for?
(In reply to Boris Zbarsky (:bz) from comment #54)
> Trevor, which DOM node did you not notifications for?

the form, and maybe a text node,  but I'm less sure about that as text nodes and there associated text leaf accessible get somewhat different handling, so the think I'd really like to know about is the form.
here's a fragment of log

from nsINode::ReplaceOrInsert()

replace or insert node in doc file:///tmp/test.html
is insert
new child tag form at 0x7fcd2fa45580
ref child tag  at (nil)
parent tag body at 0x7fcd2c8708f0

I'm not really sure what this xul stuff is doing here, but this was logged in DocAccessible::ContentRemoved(nsIContent *, nsIContent *)

baraccessible tree removal update for doc file:///tmp/test.html
container content at 0x7fcd2c6effb0 tag slider
removal of child 0x7fcd2c6f0040 tag thumb

This should be happening in the next willRefresh() after the content change logged in Accessible::AppendChild()

accessible child appended in doc file:///tmp/test.html
parent accessible 0x7fcd3022f300 node 0x7fcd2f128000 tag #document
child 0x7fcd2c6eaed0  index 2 node 0x7fcd2fa45580 tag form

here's where why find the existing one that was never removed logged in Accessible::RemoveChild()

I don't think that adds too much, but it may be useful.
###!!! ASSERTION: Adopting child!: 'Error', file /src/moz2/accessible/src/generic/Accessible.cpp, line 2464
accessible removed in doc file:///tmp/test.html
parent 0x7fcd2c747f80 node 0x7fcd2c871280 tag option
removed child 0x7fcd2c6eaed0 node 0x7fcd2fa45580 tag form
So in that testcase, the <form> is first appended as a child of an <option> and then removed from there and appended as a child of the <body>.  The remove actually happens as part of the document.body.appendChild(formNode) call.

So maybe start there?  Put a dump() call right before that call, breakpoint in nsGlobalWindow::Dump, then when that gets hit breakpoint in nsCSSFrameConstructor::ContentRemoved and see what happens on the next call to it (which should have aChild being the form).  That's the method that would normally call nsAccessibilityService::ContentRemoved.
(In reply to Boris Zbarsky (:bz) from comment #57)
> So in that testcase, the <form> is first appended as a child of an <option>
> and then removed from there and appended as a child of the <body>.  The
> remove actually happens as part of the document.body.appendChild(formNode)
> call.

yeah, I'd figured that part out by poking around in nsInode.cpp

> So maybe start there?  Put a dump() call right before that call, breakpoint
> in nsGlobalWindow::Dump, then when that gets hit breakpoint in
> nsCSSFrameConstructor::ContentRemoved and see what happens on the next call
> to it (which should have aChild being the form).  That's the method that
> would normally call nsAccessibilityService::ContentRemoved.

ok, that's the part I wanted :)  I wasn't really clear on if that function always got called.  how the frame tree gets updated isn't something I've looked at much yet.
Well, it doesn't _always_ get called.  But if the document has a presshell, it will be called.
(In reply to Boris Zbarsky (:bz) from comment #59)
> Well, it doesn't _always_ get called.  But if the document has a presshell,
> it will be called.

ok, sure.  That's good to know, but I don't think we should ever have a doc accessible or any children for a document that has no pres shell.
> So maybe start there?  Put a dump() call right before that call, breakpoint
> in nsGlobalWindow::Dump, then when that gets hit breakpoint in
> nsCSSFrameConstructor::ContentRemoved and see what happens on the next call
> to it (which should have aChild being the form).  That's the method that
> would normally call nsAccessibilityService::ContentRemoved.

Ok, I tried that here's what I found
nsCSSFrameConstructor::ContentRemoved() is not called after the form is removed and appeneded to the body, in fact the last time its called before the assert is to remove the select.

I saw nsCSSFrameConstructor::ContentRemoved() is sometimes called by PresShell::ContentRemoved() which can be called from nsNodeUtils::ContentRemoved() I walked through nsNodeUtils::ContentRemoved() for the <form> which seems to mostly be a wrapper for the IMPL_MUTATION_NOTIFICATION() macro which basically just calls a function on all of node->mSlots->mMutationObservers.  I see that that array is empty for the <form> I'm going to guess it should probably contain the pres shell?
IMPL_MUTATION_NOTIFICATIONS walks the parent chain, so should be calling ContentRemoved() on all observers of all ancestors.  And the presshell should be observing the document.

This is when I ask whether all of the bits of the testcase are really needed...  Are they?  reducing the script to the minimal number of DOM operations possible might help us see what's going on here.
(In reply to Boris Zbarsky (:bz) from comment #62)
> IMPL_MUTATION_NOTIFICATIONS walks the parent chain, so should be calling
> ContentRemoved() on all observers of all ancestors.  And the presshell
> should be observing the document.

ok, oops

> This is when I ask whether all of the bits of the testcase are really
> needed...  Are they?  reducing the script to the minimal number of DOM
> operations possible might help us see what's going on here.

Then this is where I ask how much time you want me to spend fidling with removing random combinations of things to see if they're not actually needed.  I know that some of those changes are certainly needed, I tried removing some operations from the original test case and was able to remove a number, but not all. its also not clear to me if all of the bits the test uses are actually needed or not.  I can try to remove more stuff tomorrow and see what happens.
OK.  So I just stopped in that appendChild call.

At that point, the form is already not in the document.  It's in a disconnected subtree whose root is a <select id="select_2">.  That subtree got removed from the DOM right here:

  secondSelect.parentNode.removeChild(secondSelect);

Specifically, the form's parent is <option id="option_4"> whose parent is <option id="option_1">, whose parent is <optgroup id="optgroup_1">, whose parent is a <fieldset>, whose parent is an <option id="option_2"> which is inside the <select id="select_2">.

In particular, during foo() the optgroup containing the option that contains the form got removed from where it used to be and plopped into the fieldset, which was then placed inside an <option> in the second select.

So what's going on with the secondSelect removal above?  Does _that_ land you in nsCSSFrameConstructor::ContentRemoved?
(In reply to Boris Zbarsky (:bz) from comment #64)
> OK.  So I just stopped in that appendChild call.
> 
> At that point, the form is already not in the document.  It's in a
> disconnected subtree whose root is a <select id="select_2">.  That subtree
> got removed from the DOM right here:
> 
>   secondSelect.parentNode.removeChild(secondSelect);
> 
> Specifically, the form's parent is <option id="option_4"> whose parent is
> <option id="option_1">, whose parent is <optgroup id="optgroup_1">, whose
> parent is a <fieldset>, whose parent is an <option id="option_2"> which is
> inside the <select id="select_2">.
> 
> In particular, during foo() the optgroup containing the option that contains
> the form got removed from where it used to be and plopped into the fieldset,
> which was then placed inside an <option> in the second select.
> 
> So what's going on with the secondSelect removal above?  Does _that_ land
> you in nsCSSFrameConstructor::ContentRemoved?

yes, it does. I just walked through it, we call accService->ContentRemoved() which ends up in DocAccessible::UpdateTree() and DocAccessible::UncacheChildrenInSubtree()  which should remove the select and all its kids mappings in the doc accessibles map from nodes to accessibles, but when its done GetAccessible(formNode) still gets the form meaning the mapping wasn't removed.  I haven't looked into how exactly that happens yet.

btw I did  fiddle with reducing the test case a little, though its not really useful anymore.
Ok, so here's what I think happens

we start with a tree like this
<select>
<optgroup>
<option>

the a11y tree flattens option kids of optgroups intho the accessible for the select so the accessible tree is
{combobox: [
  {selectList: [
    {optgrup: [] }
    {option: [] }
  }}

then we insert the form, and then move the <optgroup> containing the <option> to another <option> in a different select, but no reflow happens in between so a11y tree is not updated.
Then we handle removing the <optgroup> from the select, DocAccessible::UpdateTree() correctly nukes the subtree for the optgroup, but misses the accessible for the option because its a sibling not a child.

Then after that removal is handled we have a accessible tree something like this
{combobox: {
  {option: [] }
}

which is bogus of course since the DOM tree is <select> </select>

then reflow happens and we go to add the <form> to the accessible for the <option> its getting put under so we have an accessible tree like this
{combobox: [
  {selectList: [
    {option: [
      {form: [] }
    }
  }
then later we remove that whole second select, but we fail to nuke the accessible for the <option> and <form> because they're in the wrong subtree.

So then we go to insert the <form> someplace else, but the old accessible still has a parent and is still "alive" which triggers the badness we see here.
Would be nice if we could avoid flattening. I'm sure Alexander has thoughts on this.
(In reply to David Bolter [:davidb] from comment #67)
> Would be nice if we could avoid flattening. I'm sure Alexander has thoughts
> on this.

yeah, I tracked doing that back to bug 278872 which added CacheOptSiblings() in jan 2005.  That bug doesn't talk about why its done that way, and I sort of suspect it was suposed to work that way before that bug but its not really clear
Btw, we were asked to stop flat hierarchies on selects but I was never about to file a bug on it. File it?
(In reply to alexander :surkov from comment #69)
> Btw, we were asked to stop flat hierarchies on selects but I was never about
> to file a bug on it. File it?

ok, that sounds nice, and really like the right fix here.

However I don't want to make that change here of course so I think I'll just make ContentRemoved() recursive over the content in the subtree somewhat ugly but I don't really have any better ideas :/
(In reply to Trevor Saunders (:tbsaunde) from comment #70)
> (In reply to alexander :surkov from comment #69)
> However I don't want to make that change here of course so I think I'll just
> make ContentRemoved() recursive over the content in the subtree somewhat
> ugly but I don't really have any better ideas :/

whenever we change insertion point then we may run into problems. We should assert I think. (btw, this bug is another usecase of bug 690417).
(In reply to alexander :surkov from comment #71)
> (In reply to Trevor Saunders (:tbsaunde) from comment #70)
> > (In reply to alexander :surkov from comment #69)
> > However I don't want to make that change here of course so I think I'll just
> > make ContentRemoved() recursive over the content in the subtree somewhat
> > ugly but I don't really have any better ideas :/
> 
> whenever we change insertion point then we may run into problems. We should

I'm not sure what you mean by that, it seems we have a problem whenever child dom content is not child of accessible

> assert I think. (btw, this bug is another usecase of bug 690417).

yeah, but I don't think we want to wait to fix this, and that doesn't automatically fix this as I see it.
(In reply to Trevor Saunders (:tbsaunde) from comment #72)

> > whenever we change insertion point then we may run into problems. We should
> 
> I'm not sure what you mean by that, it seems we have a problem whenever
> child dom content is not child of accessible

that's what I meant, just stated it generally when DOM structure doesn't corresponds to a11y structure.

> > assert I think. (btw, this bug is another usecase of bug 690417).
> 
> yeah, but I don't think we want to wait to fix this, and that doesn't
> automatically fix this as I see it.

It doesn't fix anything automatically of course, it was supposed to be a big reorg that should have made things like this impossible. Of course all of this was in my mind and bug deoesn't contain any good instructions. And of course again we shouldn't wait to fix this bug.

If you fix hierarchy thing before this one then add an assertion to catch other cases.
Attached patch patch (obsolete) β€” β€” Splinter Review 
I don't really like this approach much, and I imagine it will make content removal somewhat slower, but the only other solution I can think of is not flattening optgroups
Attachment #678472 - Flags: review?(surkov.alexander)
(In reply to Trevor Saunders (:tbsaunde) from comment #74)
> Created attachment 678472 [details] [diff] [review]
> patch
> 
> I don't really like this approach much, and I imagine it will make content
> removal somewhat slower,

that's crazy approach, it's double processing in 99% cases and it makes us fire and coalesce events on on.

> but the only other solution I can think of is not
> flattening optgroups

that's what we should do I think
(In reply to alexander :surkov from comment #75)
> (In reply to Trevor Saunders (:tbsaunde) from comment #74)
> > Created attachment 678472 [details] [diff] [review]
> > patch
> > 
> > I don't really like this approach much, and I imagine it will make content
> > removal somewhat slower,
> 
> that's crazy approach, it's double processing in 99% cases and it makes us
> fire and coalesce events on on.

so, I was hopeing we'd just remove this stuff when we did below, so this would be a temporary solution.

> > but the only other solution I can think of is not
> > flattening optgroups
> 
> that's what we should do I think

my concerns with this are
1 do we want to make a behavior change in a bug not everyone can see?
2. do we want to come up with something we can take on branches?
(In reply to Trevor Saunders (:tbsaunde) from comment #76)
> (In reply to alexander :surkov from comment #75)
> > (In reply to Trevor Saunders (:tbsaunde) from comment #74)
> > > Created attachment 678472 [details] [diff] [review]
> > > patch
> > > 
> > > I don't really like this approach much, and I imagine it will make content
> > > removal somewhat slower,
> > 
> > that's crazy approach, it's double processing in 99% cases and it makes us
> > fire and coalesce events on on.
> 
> so, I was hopeing we'd just remove this stuff when we did below, so this
> would be a temporary solution.

I guess we can do something very hacky like if (aContainer->IsHTMLSelect()) {
  do stuff specific for selects();
}

I'm not sure that fixes all cases but I believe it fixes this one.
(In reply to Trevor Saunders (:tbsaunde) from comment #76)

> > > but the only other solution I can think of is not
> > > flattening optgroups
> > 
> > that's what we should do I think
> 
> my concerns with this are
> 1 do we want to make a behavior change in a bug not everyone can see?

no, we need a new bug (you may not put into relation with this one).

> 2. do we want to come up with something we can take on branches?

If we had a nice workaround of the issue then yes. Otherwise we can probably live with the bug for a while. Where we regressed, is it from Firefox 4 timeframe? Are there any ο»Ώconsiderations how bad can it be?
Btw, I filed bug 809338 for hierarchical selects
(In reply to alexander :surkov from comment #78)
> (In reply to Trevor Saunders (:tbsaunde) from comment #76)
> > 2. do we want to come up with something we can take on branches?
> 
> If we had a nice workaround of the issue then yes. Otherwise we can probably
> live with the bug for a while. Where we regressed, is it from Firefox 4
> timeframe? Are there any ο»Ώconsiderations how bad can it be?

I *believe* it was present in firefox 4 and possibly before, but its not exactly trivial to check that, so who knows.
(In reply to Trevor Saunders (:tbsaunde) from comment #77)

> I guess we can do something very hacky like if (aContainer->IsHTMLSelect()) {
>   do stuff specific for selects();
> }

yep, like if it's optgroup element then do UpdateTree for all its child option elements.

> I'm not sure that fixes all cases but I believe it fixes this one.

agree, special case is ok if bug 809338 cannot be fixed in reasonable time (for example, if we break AT).
Attachment #678472 - Flags: review?(surkov.alexander)
Merge happens at the beginning of next week. The current plan is to fix via the 'unflatten' bug 809338 and that should land when trunk is FF 20 at least. We can assess what to do for branches then.

(Alexander, I'm curious for your thoughts on comment 77)
Comment on attachment 678472 [details] [diff] [review]
patch

throwing an r- on here to reflect later comments in the bug (and per davidb) saying we need another patch. Otherwise this gives me false hope :-)
Attachment #678472 - Flags: review-
(In reply to David Bolter [:davidb] from comment #82)
> Merge happens at the beginning of next week. The current plan is to fix via
> the 'unflatten' bug 809338 and that should land when trunk is FF 20 at
> least. We can assess what to do for branches then.
> 
> (Alexander, I'm curious for your thoughts on comment 77)

I answered it in comment 81. We need workaround here. AT has problems with not flat hierarchy in bug 809338. Insertion point bug 690417 (which should solve this bug) is not a short term.
> I answered it in comment 81. We need workaround here. AT has problems with
> not flat hierarchy in bug 809338. Insertion point bug 690417 (which should
> solve this bug) is not a short term.

so, I guess we can do that, but I'm only 90% sure it will actually work for all cases.  So I think we should decide on a point in the near future when we'll remove the backwards compat mode with flat selects.
Attached patch patch (obsolete) β€” β€” Splinter Review 

  

  



        Attachment #682312 -
        Flags: review?(surkov.alexander)

    
    

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

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

please file an independent bug add mochitest there

this logic should be part of DocAccessible::UpdateTree, it lets you save time for reorder event coalescence and searching for alert containers.

::: accessible/src/generic/DocAccessible.cpp
@@ +1459,5 @@
>    // null (document element is removed).
>    Accessible* container = aContainerNode ?
>      GetAccessibleOrContainer(aContainerNode) : this;
>  
> +  // XXX selects shouldn't be flat in the first place

nit: dot in the end. refer to bug

@@ +1461,5 @@
>      GetAccessibleOrContainer(aContainerNode) : this;
>  
> +  // XXX selects shouldn't be flat in the first place
> +  if (aContainerNode->IsHTML(nsGkAtoms::select) && aChildNode->IsHTML(nsGkAtoms::optgroup)) {
> +    for (nsIContent* childOptionNode = aChildNode->GetFirstChild();

childOptionNode -> optionNode, it's shorter and seems to be clear

@@ +1462,5 @@
>  
> +  // XXX selects shouldn't be flat in the first place
> +  if (aContainerNode->IsHTML(nsGkAtoms::select) && aChildNode->IsHTML(nsGkAtoms::optgroup)) {
> +    for (nsIContent* childOptionNode = aChildNode->GetFirstChild();
> +         childOptionNode; 

nit: whitespace

@@ +1463,5 @@
> +  // XXX selects shouldn't be flat in the first place
> +  if (aContainerNode->IsHTML(nsGkAtoms::select) && aChildNode->IsHTML(nsGkAtoms::optgroup)) {
> +    for (nsIContent* childOptionNode = aChildNode->GetFirstChild();
> +         childOptionNode; 
> +         childOptionNode = childOptionNode->GetNextSibling()) {

it makes sense to assert if container != getAccessible(optionNode)->Parent() in case that hierarchy bug was fixed but we forgot to remove this hack.

@@ +1468,5 @@
> +      if (childOptionNode->IsHTML(nsGkAtoms::option))
> +        UpdateTree(container, childOptionNode, false);
> +
> +      if (childOptionNode->IsHTML(nsGkAtoms::optgroup))
> +        ContentRemoved(aContainerNode, childOptionNode);

optgroups can't be nested

  

  



        Attachment #682312 -
        Flags: review?(surkov.alexander)

    
    

    
  
> @@ +1463,5 @@
> > +  // XXX selects shouldn't be flat in the first place
> > +  if (aContainerNode->IsHTML(nsGkAtoms::select) && aChildNode->IsHTML(nsGkAtoms::optgroup)) {
> > +    for (nsIContent* childOptionNode = aChildNode->GetFirstChild();
> > +         childOptionNode; 
> > +         childOptionNode = childOptionNode->GetNextSibling()) {
> 
> it makes sense to assert if container != getAccessible(optionNode)->Parent()
> in case that hierarchy bug was fixed but we forgot to remove this hack.

well, its not that simple because of having two accessibles for <select> in case of combobox right?

> @@ +1468,5 @@
> > +      if (childOptionNode->IsHTML(nsGkAtoms::option))
> > +        UpdateTree(container, childOptionNode, false);
> > +
> > +      if (childOptionNode->IsHTML(nsGkAtoms::optgroup))
> > +        ContentRemoved(aContainerNode, childOptionNode);
> 
> optgroups can't be nested

yeah, but for some reason I remembered cacheOptSiblings() trying to recurse anyway, and I wanted to stay as similar to the reverse of that logic as possible, but it seems CacheOptSiblings() doesn't actually do that atleast anymore.

  

  



    
    

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

> > it makes sense to assert if container != getAccessible(optionNode)->Parent()
> > in case that hierarchy bug was fixed but we forgot to remove this hack.
> 
> well, its not that simple because of having two accessibles for <select> in
> case of combobox right?

assertion would be good, implementation may vary

  

  



    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch v3
        β€” 
          β€” Splinter Review
      

    


  

  

  



        Attachment #678472 -
        Attachment is obsolete: true

        Attachment #682312 -
        Attachment is obsolete: true

        Attachment #682830 -
        Flags: review?(surkov.alexander)

    
    

    
  
Comment on attachment 682830 [details] [diff] [review]
patch v3

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

Why wouldn't you run just UpdateTreeInternal for each option to avoid code copy/paste (i.e. putting this code into UpdateTree())?

  

  



    
    

    
  
(In reply to alexander :surkov from comment #91)
> Comment on attachment 682830 [details] [diff] [review]
> patch v3
> 
> Review of attachment 682830 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Why wouldn't you run just UpdateTreeInternal for each option to avoid code
> copy/paste (i.e. putting this code into UpdateTree())?

because it would we doing some of the processing for alert accessibles on each option which you complained about in comment 87

  

  



    
    

    
  
(In reply to Trevor Saunders (:tbsaunde) from comment #92)
> > Why wouldn't you run just UpdateTreeInternal for each option to avoid code
> > copy/paste (i.e. putting this code into UpdateTree())?
> 
> because it would we doing some of the processing for alert accessibles on
> each option which you complained about in comment 87

nah, I complained about up traversal for alerts and reorder events coalescence. There's no any alert processing inside UpdateTreeInternal (all we have aChild->ARIARole() == roles::MENUPOPUP and it shouldn't hurt us).

Options node support is edge and temporary case so I'd prefer to have code nicer rather than slighty optimized.

  

  



    
    

    
  
(In reply to alexander :surkov from comment #93)
> (In reply to Trevor Saunders (:tbsaunde) from comment #92)
> > > Why wouldn't you run just UpdateTreeInternal for each option to avoid code
> > > copy/paste (i.e. putting this code into UpdateTree())?
> > 
> > because it would we doing some of the processing for alert accessibles on
> > each option which you complained about in comment 87
> 
> nah, I complained about up traversal for alerts and reorder events
> coalescence. There's no any alert processing inside UpdateTreeInternal (all
> we have aChild->ARIARole() == roles::MENUPOPUP and it shouldn't hurt us).
> 
> Options node support is edge and temporary case so I'd prefer to have code
> nicer rather than slighty optimized.

I don't really care much either way, but I'd rather not spend too much time polishing temporary code.

  

  



    
    

    
  
So put it under UpdateTree, it's plain and simple

  

  



    
    

    
  
(In reply to alexander :surkov from comment #95)
> So put it under UpdateTree, it's plain and simple

I'm not actually convinced its really any nicer after looking at it for a few minutes, and even if it is I'm not convinced its worth spending the time on code that is temporary.  That said if you really think its better write a patch and make me review it :)

  

  



    
    

    
  


  
that temporary code is really for a while :)

  

  



        Attachment #683851 -
        Flags: review?(trev.saunders)

    
    

    
  
Comment on attachment 683851 [details] [diff] [review]
patch: what I kept in mind

to be honest I think I like my approach better, but I guess this is fine, considering it should only be used in the backwards compat case very shortly.

  

  



        Attachment #683851 -
        Flags: review?(trev.saunders) → review+

    
    

    
  
(In reply to Trevor Saunders (:tbsaunde) from comment #98)
> Comment on attachment 683851 [details] [diff] [review]
> patch: what I kept in mind
> 
> to be honest I think I like my approach better, but I guess this is fine,

it's a middle between your approaches, hopefully a gold one ;)

1) calling UpdateTree() gives us unnecessary perf hit.
2) keeping it inside UpdateTree() is just another special case (like tree walking in case when content is not accessible)
3) keeping it inside UpdateTreeInternal() is a little faster than 2 but makes us operate on mutation and reorder events. I'd like to reduce those to not spread a logic through the code when we can do that.

That's why I preferred 2 over 1 and 3.

  

  



    
    

    
  
http://hg.mozilla.org/integration/mozilla-inbound/rev/03b7416c3836

  

  



        Attachment #682830 -
        Flags: review?(surkov.alexander)

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/03b7416c3836

  

  


Status: NEW → RESOLVED
Closed: 12 years ago

          status-firefox16:
          affected → wontfix

          status-firefox17:
          affected → wontfix

          status-firefox19:
          --- → affected

          status-firefox20:
          --- → fixed

          status-firefox-esr17:
          --- → affected
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla20

    
    

    
  
We need branch patches (and approval requests) for this bug for all of
  ESR-10
  ESR-17
  aurora (19)
  beta (18)

Also, please in the future request sec-approval? on patches to security bugs before landing so we can look at our branch exposure before landing.

  

  



          status-firefox-esr10:
          --- → affected

          tracking-firefox-esr10:
          --- → 18+

          tracking-firefox-esr17:
          --- → 18+

    
    

    
  
Trev, would you like to finish it (comment #102)?

  

  



    
    

    
  
Comment on attachment 683851 [details] [diff] [review]
patch: what I kept in mind

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: 
Fix Landed on Version:
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch: 

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: 
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky):

[Approval Request Comment]
Bug caused by (feature/regressing bug #): its probably been around for years.
User impact if declined: out of bounds access of an object pointer to a possibly bogus pointer.  which may be exploitable or lead to crashes.
Testing completed (on m-c, etc.): yes
Risk to taking this patch (and alternatives if risky): not insignificant, but its the most minimal, and contained thing we can do.
String or UUID changes made by this patch: none

  

  



        Attachment #683851 -
        Flags: approval-mozilla-release?

        Attachment #683851 -
        Flags: approval-mozilla-esr10?

        Attachment #683851 -
        Flags: approval-mozilla-beta?

        Attachment #683851 -
        Flags: approval-mozilla-aurora?

    
    

    
  
(In reply to alexander :surkov from comment #103)
> Trev, would you like to finish it (comment #102)?

I'm not sure what you want me to do here other than ask for approvals, and take care of landing it for you.

  

  



    
    

    
  
Comment on attachment 683851 [details] [diff] [review]
patch: what I kept in mind

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: 
Fix Landed on Version:
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch: 

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

  

  



        Attachment #683851 -
        Flags: approval-mozilla-esr17?

    
    

    
  
Comment on attachment 683851 [details] [diff] [review]
patch: what I kept in mind

Approving for all unreleased versions of Firefox, given this is an sg:crit with manageable risk and we're in the third week of the cycle.

  

  



        Attachment #683851 -
        Flags: approval-mozilla-release?

        Attachment #683851 -
        Flags: approval-mozilla-release-

        Attachment #683851 -
        Flags: approval-mozilla-esr17?

        Attachment #683851 -
        Flags: approval-mozilla-esr17+

        Attachment #683851 -
        Flags: approval-mozilla-esr10?

        Attachment #683851 -
        Flags: approval-mozilla-esr10+

        Attachment #683851 -
        Flags: approval-mozilla-beta?

        Attachment #683851 -
        Flags: approval-mozilla-beta+

        Attachment #683851 -
        Flags: approval-mozilla-aurora?

        Attachment #683851 -
        Flags: approval-mozilla-aurora+

    
    

    
  
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/32bb1429c7c1

  

  



    
    

    
  
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/e5f68de5747c

  

  



    
    

    
  
remote:   https://hg.mozilla.org/releases/mozilla-esr17/rev/afee6cd6b5b6

  

  



    
    

    
  
remote:   https://hg.mozilla.org/releases/mozilla-esr10/rev/105b76710102

  

  



    
    

    
  
(In reply to Trevor Saunders (:tbsaunde) from comment #105)
> (In reply to alexander :surkov from comment #103)
> > Trev, would you like to finish it (comment #102)?
> 
> I'm not sure what you want me to do here other than ask for approvals, and
> take care of landing it for you.

yes, nothing else, thank you. technically you was an assignee so I though you could finish it (also I know it sounds funny but it seems I don't have enough room on my HDD to keep those repos).

  

  



    
    

    
  
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/735a7bf28a89

  

  



    
    

    
  
remote:   https://hg.mozilla.org/releases/mozilla-esr10/rev/45d7f91719da

  

  



    
    

    
  
remote:   https://hg.mozilla.org/releases/mozilla-esr17/rev/b8603c6f970b

  

  



    
    

    
  
> yes, nothing else, thank you. technically you was an assignee so I though
> you could finish it (also I know it sounds funny but it seems I don't have
> enough room on my HDD to keep those repos).

sounds we should buy you a bigger / another drive :)

btw I don't actually have seperate repos, but that may be as much a pain in the but as it is useful.

  

  


Whiteboard: [adv-main18+][adv-esr17+][adv-esr10+]

    
    

    
  
Reported against Fennec but also tracking for desktop. I'm not able to repro on any desktop build, and according to comment 0, Christian can't either. I'd like to ask Christian if he doesn't mind verifying this fix against the config on which he found it.

  

  



    
    

    
  
(In reply to Matt Wobensmith from comment #117)

> I'd like to ask Christian if he doesn't mind verifying this fix against the
> config on which he found it.

That config doesn't exist anymore right now.

  

  



    
    

    
  
Can you confirm that the issue doesn't happen on a new build of Fennec?

  

  


Group: core-security

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: