Closed Bug 1088148 Opened 10 years ago Closed 10 years ago

[e10s] crash in mozilla::a11y::DocAccessibleParent::AddChildDoc(mozilla::a11y::DocAccessibleParent*, unsigned __int64)

Categories

(Core :: DOM: Content Processes, defect)

Product:
Core
Component:
DOM: Content Processes
Version:
36 Branch
Platform:
x86
Windows NT
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla36
Tracking Flags:
Tracking Status
e10s m5+ ---
firefox35 + wontfix
firefox36 + fixed
firefox37 + fixed

People

(Reporter: jbecerra, Assigned: tbsaunde)

References

Details

(Keywords: crash, topcrash, Whiteboard: maybe not e10s? see comment 14)

Crash Data

Attachments

(4 files)

Notify the main process of new child docs after firing events
4.21 KB, patch
davidb
: review+
Details | Diff | Splinter Review
only send ipc accessibility messages for content child processes
846 bytes, patch
davidb
: review+
Details | Diff | Splinter Review
disable ipc accessibility messages for now to make crashes go away
649 bytes, patch
davidb
: review+
Details | Diff | Splinter Review
only tell the parent process about child documents that are attached to their parent
2.72 KB, patch
surkov
: review+
Details | Diff | Splinter Review 
[Tracking Requested - why for this release]:

[Tracking Requested - why for this release]:

This bug was filed from the Socorro interface and is 
report bp-1f9ab2e1-4385-425c-9ea6-1e20b2141022.
=============================================================

This is currently #8 in the list of top crashers for Fx36, and it is also showing high in the list of explosive reports. It's mostly happening on Windows 8.1 and Windows 7, and there are several comments in the reports, mostly saying they were loading a tab, perhaps with Flash. This has been around since 10/01 but there was a big spike on 10/22.

There are a few comments about people using their browser in e10s mode and seeing the crash reporter come up, but no links to crashes in about:crashes. This might be related to bug 1087410, which was just fixed, but it's just a guess.

There was no correlation data for add-ons.

More reports at: https://crash-stats.mozilla.com/report/list?product=Firefox&signature=mozilla%3A%3Aa11y%3A%3ADocAccessibleParent%3A%3AAddChildDoc%28mozilla%3A%3Aa11y%3A%3ADocAccessibleParent%2A%2C+unsigned+__int64%29

0 	xul.dll 	mozilla::a11y::DocAccessibleParent::AddChildDoc(mozilla::a11y::DocAccessibleParent*, unsigned __int64) 	accessible/ipc/DocAccessibleParent.h
1 	xul.dll 	mozilla::dom::ContentParent::RecvPDocAccessibleConstructor(mozilla::a11y::PDocAccessibleParent*, mozilla::a11y::PDocAccessibleParent*, unsigned __int64 const&) 	dom/ipc/ContentParent.cpp
2 	xul.dll 	mozilla::dom::PContentParent::OnMessageReceived(IPC::Message const&) 	obj-firefox/ipc/ipdl/PContentParent.cpp
3 	xul.dll 	mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) 	ipc/glue/MessageChannel.cpp
4 	xul.dll 	mozilla::ipc::MessageChannel::DispatchMessageW(IPC::Message const&) 	ipc/glue/MessageChannel.cpp
5 	xul.dll 	mozilla::ipc::MessageChannel::OnMaybeDequeueOne() 	ipc/glue/MessageChannel.cpp
6 	xul.dll 	MessageLoop::RunTask(Task*) 	ipc/chromium/src/base/message_loop.cc
7 	xul.dll 	MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask const&) 	ipc/chromium/src/base/message_loop.cc
8 	xul.dll 	MessageLoop::DoWork() 	ipc/chromium/src/base/message_loop.cc
9 	xul.dll 	mozilla::ipc::DoWorkRunnable::Run() 	ipc/glue/MessagePump.cpp
10 	xul.dll 	nsThread::ProcessNextEvent(bool, bool*) 	xpcom/threads/nsThread.cpp
11 	xul.dll 	NS_ProcessNextEvent(nsIThread*, bool) 	xpcom/glue/nsThreadUtils.cpp
12 	xul.dll 	mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp
13 	xul.dll 	MessageLoop::RunHandler() 	ipc/chromium/src/base/message_loop.cc
14 	xul.dll 	MessageLoop::Run() 	ipc/chromium/src/base/message_loop.cc
15 	xul.dll 	nsBaseAppShell::Run() 	widget/xpwidgets/nsBaseAppShell.cpp
16 	xul.dll 	nsAppShell::Run() 	widget/windows/nsAppShell.cpp
17 	xul.dll 	nsAppStartup::Run() 	toolkit/components/startup/nsAppStartup.cpp
18 	xul.dll 	XREMain::XRE_mainRun() 	toolkit/xre/nsAppRunner.cpp
19 	xul.dll 	XREMain::XRE_main(int, char** const, nsXREAppData const*) 	toolkit/xre/nsAppRunner.cpp
20 	xul.dll 	XRE_main 	toolkit/xre/nsAppRunner.cpp
21 	firefox.exe 	do_main 	browser/app/nsBrowserApp.cpp
22 	firefox.exe 	NS_internal_main(int, char**) 	browser/app/nsBrowserApp.cpp
23 	firefox.exe 	wmain 	toolkit/xre/nsWindowsWMain.cpp
24 	firefox.exe 	__tmainCRTStartup 	f:/dd/vctools/crt/crtw32/startup/crt0.c:255
25 	kernel32.dll 	BaseThreadInitThunk 	
26 	ntdll.dll 	__RtlUserThreadStart 	
27 	ntdll.dll 	_RtlUserThreadStart
I had a few of these crashes today.

For me, I was in e10s mode. The crashes happened when I opened https://sheriffs.etherpad.mozilla.org/sheriffing-notes? in a tab. I then went to file a bug about this crash, so I clicked the "File a bug" link in the crash report in about:crashes, and then Firefox crashed again.

(Now that I've disabled e10s mode, loading that tab no longer crashes.)
I see this crash when capturing a screen shot with snagit ( http://www.techsmith.com/snagit.html )

Snagit Version V10.0.1 

Steps to reproduce:
Install Snagit
Open Firefox
Press the print screen button to capture the screen.
Snagit will freeze the screen and its capture selector will appear
Capture Screen or press escape (same result)
Accessibility prompt will appear
Moments later Firefox crashes.

Expected Result:
No disable e10s because of accessibility prompt
Screen capture (with snagit)
No Crash

Crash Reports :
https://crash-stats.mozilla.com/report/index/7828d85c-abe9-4891-ad24-11ee52141023
https://crash-stats.mozilla.com/report/index/590e6899-34ea-43ed-ad3b-aba9e2141023
https://crash-stats.mozilla.com/report/index/157d90e1-6202-4b78-b538-2f0942141023
(plus several more with the same signature)
BuildIDMozilla/5.0 (Windows NT 6.1; Win64; x64; rv:36.0) Gecko/20100101 Firefox/36.0 ID:20141023030203 CSet: 88adcf8fef83

Reproducible: Always on above Build ID
Just re-tested with the current Snagit V12 trial, crash is still the same.
(In reply to martin from comment #2)
> I see this crash when capturing a screen shot with snagit (
> http://www.techsmith.com/snagit.html )
> 
> Snagit Version V10.0.1 
> 
> Steps to reproduce:
> Install Snagit
> Open Firefox
> Press the print screen button to capture the screen.
> Snagit will freeze the screen and its capture selector will appear
> Capture Screen or press escape (same result)
> Accessibility prompt will appear

I guess it uses an accessibility API for something.

> Moments later Firefox crashes.
> 
> Expected Result:
> No disable e10s because of accessibility prompt

e10s and accessibility isn't a supported configuration atm, so if snagit uses an accessibility API then that is expected.

> Screen capture (with snagit)
> No Crash

Given your using a unsupported configuration that's kind of expected.  However it seems like a lot of people are using this config even though the prompt is there exactly because they shouldn't :(
It looks like this crash is caused by us not always keeping the accessible tree in the parent up to date with the one in the child.  When we tell the parent about the new doc we've necessarily have a outerDoc accessible in the child, but the crash looks to be coming from there being no cooresponding accessible for the outerDoc in the parent.
Dump has finished uploading.

I did not enable accessibility, just taking screenshots with snagit.
So maybe the code detecting acessability is giving false positives?
Sorry no dump available, (for this bug).
(In reply to martin from comment #7)
> Sorry no dump available, (for this bug).

a core dump wouldn't help much anyway, though a minimal web page would be very nice.

> I did not enable accessibility, just taking screenshots with snagit.
> So maybe the code detecting acessability is giving false positives?

no, on windows it detects people sending WM_GETOBJECT messages, which afaik aare only used for accessibility
Just got this crash with e10s *disabled* on Firefox 64-bit build.
I still have e10s disabled, in theory shouldn't have any accessibility features enabled, but I do use a Wacom tablets, which sounds like it could be involved.
I also am now running with  accessibility.force_disabled==-1  but e10s at least still thinks accessibility is enabled.
Oops, just to clarify, the above comments are with regard to Firefox config before the last crash matching this signature ( https://crash-stats.mozilla.com/report/index/06c3ad1d-d71b-465d-8069-68df32141025 )
(In reply to Rob North from comment #10)

> I also am now running with  accessibility.force_disabled==-1  but e10s at
> least still thinks accessibility is enabled.

the pref is rather strange since it's about platform accessibility only but do I understand right that you have -1 value which means "force enabled" which means the accessibility is not disabled.
Ok, right. Was reading documentation from when this was a boolean property!
Now set to "1" will see if this improves behaviour.

Note, when I had the first few crashes, had the property set to it's default: 0.

Since I couldn't even start firefox without this bug happening with property set to -1 and now I can, I'm assuming that the property already has some impact. Will leave it in disabled state and see what happens.
My browser (nightly) has been unusable due to this crash soon after startup on Win 8.1 the last couple of days. I am NOT using e10s because it breaks add-ons I'm using. When I went to set the accessibility.force-disabled pref after reading comments here I noticed I had set the pref accessibility.typeaheadfind.flashBar to 0 (can't remember why or what that does). the Force disabled pref so far seems to have stopped the crashes.

I did NOT experience the crashes on my main win 7 machine, but since I'm at a conference I can't check whether I had set the accessibility.typeaheadfind.flashBar pref in that profile (is that a sync'd pref? if so then I probably did). Because my main machine is the non-crashing win 7 machine I don't know when this started crashing on the win 8.1 tablet-y thing I take to conferences.
Whiteboard: maybe not e10s? see comment 14
Sadly I'm still getting the crash, but I had a nice 15 minutes without any. False hope.

Some websites seem to make the crashes happen more often, not necessarily on load but at some interval. http://cowl.ws/ was one that triggered crashes for me -- was crashing soon after startup until I pulled that one out of my sessionrestore list.
(In reply to Daniel Veditz [:dveditz] from comment #14)
> My browser (nightly) has been unusable due to this crash soon after startup
> on Win 8.1 the last couple of days. I am NOT using e10s because it breaks
> add-ons I'm using. When I went to set the accessibility.force-disabled pref

it doesn't really make sense you'd see this without e10s, this code shouldn't run at all there.  is any kind of child process around?

> after reading comments here I noticed I had set the pref
> accessibility.typeaheadfind.flashBar to 0 (can't remember why or what that

I'm pretty sure that's unrelated and just effects some UI or layout stuff.


(In reply to Daniel Veditz [:dveditz] from comment #15)
> Sadly I'm still getting the crash, but I had a nice 15 minutes without any.
> False hope.

that also doesn't make much sense, if you set that pref to 1 then there really shouldn't be any a11y code running much less this ipc stuff.
I can confirm that the (already slightly bit-rotted) patch resolves the issue on linux x86_64 trunk using e10s for me.  Specifically, running a mochitest with "mach mochitest-plain --e10s PATH" with a locally built firefox x86_64 build (both non-debug and debug) on Ubuntu 14.04.1 LTS using gnome-shell 3.10.4-0-ubuntu5.2 was segfaulting with this error before it actually managed to start running my mochitest.  (Note that the M-e10s try server runs did not experience this problem, but I'm assuming they have a less exciting desktop environment running.)
Comment on attachment 8512862 [details] [diff] [review]
Notify the main process of new child docs after firing events

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

::: accessible/base/NotificationController.cpp
@@ +284,5 @@
> +      auto contentChild = dom::ContentChild::GetSingleton();
> +      DocAccessibleChild* parentIPCDoc = mDocument->IPCDoc();
> +      uint64_t id = reinterpret_cast<uintptr_t>(childDoc->Parent()->UniqueID());
> +      contentChild->SendPDocAccessibleConstructor(ipcDoc, parentIPCDoc,
> +          id);

nit: just move 'id' up to the previous line.

::: accessible/ipc/DocAccessibleChild.cpp
@@ +23,5 @@
>  
> +  // OuterDocAccessibles are special because we don't want to serialize the
> +  // child doc here, we'll call PDocAccessibleConstructor in
> +  // NotificationController.
> +  if (childCount == 1 && aRoot->GetChildAt(0)->IsDoc())

Is this guaranteed to be a OuterDocAccessible?
Attachment #8512862 - Flags: review?(dbolter) → review+
(In reply to David Bolter [:davidb] from comment #19)
> Comment on attachment 8512862 [details] [diff] [review]
> Notify the main process of new child docs after firing events
> 
> Review of attachment 8512862 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/base/NotificationController.cpp
> @@ +284,5 @@
> > +      auto contentChild = dom::ContentChild::GetSingleton();
> > +      DocAccessibleChild* parentIPCDoc = mDocument->IPCDoc();
> > +      uint64_t id = reinterpret_cast<uintptr_t>(childDoc->Parent()->UniqueID());
> > +      contentChild->SendPDocAccessibleConstructor(ipcDoc, parentIPCDoc,
> > +          id);
> 
> nit: just move 'id' up to the previous line.
> 
> ::: accessible/ipc/DocAccessibleChild.cpp
> @@ +23,5 @@
> >  
> > +  // OuterDocAccessibles are special because we don't want to serialize the
> > +  // child doc here, we'll call PDocAccessibleConstructor in
> > +  // NotificationController.
> > +  if (childCount == 1 && aRoot->GetChildAt(0)->IsDoc())
> 
> Is this guaranteed to be a OuterDocAccessible?

I believe so, and even if it somehow isn't we still don't want to serialize the document.
looks like mcmerge missed this somehow
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
This is still happening in recent builds:
bp-a6909294-bb9e-4ed1-816a-be6252141104
bp-698fc514-eb4f-430f-b797-cfc842141104

I had HTML5 YouTube in one tab, the new tab page in another, and I closed/restored the browser.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Crash-stats doesn't see a decline in volume either.
(In reply to David Major [:dmajor] (UTC+13) from comment #24)
> Crash-stats doesn't see a decline in volume either.

hm, it certainly fixed the crash I saw locally, but I'm willing to believe other bugs could lead to the same crash.


(In reply to David Major [:dmajor] (UTC+13) from comment #23)
> This is still happening in recent builds:
> bp-a6909294-bb9e-4ed1-816a-be6252141104
> bp-698fc514-eb4f-430f-b797-cfc842141104
> 
> I had HTML5 YouTube in one tab, the new tab page in another, and I
> closed/restored the browser.

ok, I'm curious why you had accessibility active and e10s enabled does the warning thing not work on windows maybe?
> ok, I'm curious why you had accessibility active and e10s enabled does the
> warning thing not work on windows maybe?
I'm pretty sure I wasn't opted into e10s. Most likely I hit this in a non-e10s case like dveditz in comment 14.
(In reply to David Major [:dmajor] (UTC+13) from comment #26)
> > ok, I'm curious why you had accessibility active and e10s enabled does the
> > warning thing not work on windows maybe?
> I'm pretty sure I wasn't opted into e10s. Most likely I hit this in a
> non-e10s case like dveditz in comment 14.

ok, I'd really love to know what is sending PDocAccessibleConstructor messages then.  I guess it could be lplugin processes, or maybe some other sort of child process, but that seems kind of unlikely.
Funny enough I can reproduce this crash almost instantly with a new profile after enabling e10s on my Asus T100 tablet with windows 10 tp. Yet on my current profile I cannot reproduce.
Just retested my case where this was triggered with snagit.exe
Works for me.
Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:36.0) Gecko/20100101 Firefox/36.0 ID:20141104030202 CSet: 5dde8ea48fef
Attachment #8518529 - Flags: review?(dbolter) → review+
Trevor ok to close?
Flags: needinfo?(tbsaunde+mozbugs)
I checked. We're still getting stacks unfortunately. E.g. ed091bef-3cba-4404-bfad-f3ca12141114
Flags: needinfo?(tbsaunde+mozbugs)
Hmm just re-tested with Snagit, still does not crash, but I get the accessibility prompt flashing and the pref browser.tabs.remote.autostart.disabled-because-using-a11y set to true...
Should I open another bug for that?
Flags: needinfo?(tbsaunde+mozbugs)
I can sort of reproduce this reliably using today's nightly on a Windows 8.1 Surface Pro machine:

1. Create a new profile and launch the nightly
2. When the prompt to disable e10s because of accessibility select "not now"
3. ctrl-l to focus on the location bar and go to, say, nytimes.com
4. ctrl-t to open a new tab - I here I notice the new tab page with tiles and a sort-of drop down explaining what the new page is about - and start typing a new url

Expected: I can continue typing the new url in step 4.

Actual: Crash

https://crash-stats.mozilla.com/report/index/bp-2666db68-8884-4ee8-87df-2e65b2141114
This is over 20% of nightly crash volume (and the volume in general is far too high right now). If there are no immediate ideas for a fix, please consider a backout while the investigation continues.

Clear spike on 20141022030202 puts the regression range as:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=29fbfc1b31aa&tochange=ae4d9b4ff2ee

This looks the most likely: 	
d6721fea9ad9	Trevor Saunders — bug 1074917 - teach atk to get states from proxies r=surkov, davidb, mrbkap
Blocks: 1074917
Assignee: nobody → tbsaunde+mozbugs
Comment on attachment 8524734 [details] [diff] [review]
disable ipc accessibility messages for now to make crashes go away

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

r=me yeah. please add a comment.

::: accessible/base/nsAccessibilityService.h
@@ +250,5 @@
>   */
>  inline bool
>  IPCAccessibilityActive()
>  {
> +return false;

Please add a comment like:
// XXX stop the bleeding bug 1088148.
Attachment #8524734 - Flags: review?(dbolter) → review+
https://hg.mozilla.org/mozilla-central/rev/6a160892b400
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
status-firefox36: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
since this is nightly/e10s related, no need to uplift to beta - if i'm misunderstanding please nominate for uplift, but my read on this is that it's a wontfix for 35.
status-firefox35: affected → wontfix
http://bit.ly/1IhwNUx indicates a crash spike in Nightly in this signature in the days after this was landed. Many of the comments indicate they were opening a new tab when the crash happened. Do you want a new bug filed?
Flags: needinfo?(tbsaunde+mozbugs)
(In reply to Marcia Knous [:marcia - use needinfo] from comment #45)
> http://bit.ly/1IhwNUx indicates a crash spike in Nightly in this signature
> in the days after this was landed. Many of the comments indicate they were
> opening a new tab when the crash happened. Do you want a new bug filed?

either way is fine.  It looks like its windows only? does it look like plugins are involved or at least loaded?
Flags: needinfo?(tbsaunde+mozbugs)
[Tracking Requested - why for this release]:

The spike in this signature is 1/3 of all our Nightly crashes in yesterday's numbers, we need to do something here, either a very fast fix today or a backout.
tracking-firefox37: --- → ?
Trevor, what's the best course of action here?
Flags: needinfo?(tbsaunde+mozbugs)
(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #48)
> Trevor, what's the best course of action here?

I can revert the significant bit temporarily, but someone needs to answer comment 46.
Flags: needinfo?(tbsaunde+mozbugs)
(In reply to Trevor Saunders (:tbsaunde) from comment #46)
> (In reply to Marcia Knous [:marcia - use needinfo] from comment #45)
> > http://bit.ly/1IhwNUx indicates a crash spike in Nightly in this signature
> > in the days after this was landed. Many of the comments indicate they were
> > opening a new tab when the crash happened. Do you want a new bug filed?
> 
> either way is fine.  It looks like its windows only? does it look like
> plugins are involved or at least loaded?

Plugins don't appear to be involved, the stacks are very basic, with a EXCEPTION_ACCESS_VIOLATION_READ crash at:

http://hg.mozilla.org/mozilla-central/annotate/57e4e9c33bef/accessible/ipc/DocAccessibleParent.cpp#l137
IRC recap: These are almost all Windows. I see a small handful of Linux reports, hard to say whether those are noise or just a smaller install base.

We don't have a good way of correlating with plugin presence.

Another interesting data point: 18% are showing e10s (DOMIPCEnabled). None of the reports have a ProcessType annotation -- meaning it's in the chrome process.
I would naively expect Windows to be where most of our a11y users are.
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #53)
> I would naively expect Windows to be where most of our a11y users are.

It's also where a lot of touchscreen users are, as Windows for some reason enables accessibility mode if a touchscreen is being used, and I used to experience this crash a lot when enabling e10s. Not sure if this suggestion will help, but are most of the crashes coming from Windows 8+ users?
> are most of the crashes coming from Windows 8+ users?

6.3.9600 	1548 	56.93 %   (Win8.1)
6.1.7601 	808 	29.72 %   (Win7SP1)
6.2.9200 	205 	7.54 %    (Win8)
6.1.7600 	63 	2.32 %    (Win7)
6.4.9841 	51 	1.88 %    (Win10)
6.4.9879 	29 	1.07 %    (Win10)
5.1.2600 	8 	0.29 %    (XP)
(In reply to David Major [:dmajor] (UTC+13) from comment #52)
> IRC recap: These are almost all Windows. I see a small handful of Linux
> reports, hard to say whether those are noise or just a smaller install base.

ok, that may kind of kill one of my theories as to how this is happening, but maybe not :/

> We don't have a good way of correlating with plugin presence.
> 
> Another interesting data point: 18% are showing e10s (DOMIPCEnabled). None
> of the reports have a ProcessType annotation -- meaning it's in the chrome
> process.

this code only runs in the chrome process, but I find it rather curious people without e10s hit this crash I'd like to understand that.


(In reply to Jim Mathies [:jimm] from comment #51)
> (In reply to Trevor Saunders (:tbsaunde) from comment #46)
> > (In reply to Marcia Knous [:marcia - use needinfo] from comment #45)
> > > http://bit.ly/1IhwNUx indicates a crash spike in Nightly in this signature
> > > in the days after this was landed. Many of the comments indicate they were
> > > opening a new tab when the crash happened. Do you want a new bug filed?
> > 
> > either way is fine.  It looks like its windows only? does it look like
> > plugins are involved or at least loaded?
> 
> Plugins don't appear to be involved, the stacks are very basic, with a
> EXCEPTION_ACCESS_VIOLATION_READ crash at:

the plugin theory is more complicated than that.  The proximal cause of the crash is that a accessible for a sub document is being attached into the parent document when there is no accessible for the content that is the parent of the sub document.  Since there is an accessible for the parent content in the child process it must be true that the parent process somehow isn't being told about the accessible in the child document.  One pluasable theory I had for how that could be happening involved subdocuments for plugins, but there might well be other causes, at this point I think I need to find a way to track down places that might not be emiting the events that are used to keep the parent process up to date.
Comment on attachment 8545414 [details] [diff] [review]
only tell the parent process about child documents that are attached to their parent

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

::: accessible/base/NotificationController.cpp
@@ +208,5 @@
>    mTextHash.Clear();
>  
>    // Bind hanging child documents.
>    uint32_t hangingDocCnt = mHangingChildDocuments.Length();
> +  nsTArray<nsRefPtr<DocAccessible>> newChildDocs;

maybe newChildDocuments to correspond to mHangingChildDocuments name
Attachment #8545414 - Flags: review?(surkov.alexander) → review+
(In reply to alexander :surkov from comment #59)
> Comment on attachment 8545414 [details] [diff] [review]
> only tell the parent process about child documents that are attached to
> their parent
> 
> Review of attachment 8545414 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/base/NotificationController.cpp
> @@ +208,5 @@
> >    mTextHash.Clear();
> >  
> >    // Bind hanging child documents.
> >    uint32_t hangingDocCnt = mHangingChildDocuments.Length();
> > +  nsTArray<nsRefPtr<DocAccessible>> newChildDocs;
> 
> maybe newChildDocuments to correspond to mHangingChildDocuments name

is that a goal? if so why?
keeping same coding style is good idea in general
(In reply to Trevor Saunders (:tbsaunde) from comment #62)
> remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/33125ece9497

it'd be good if you were addressed comments before landing

> remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/06ea411856f7

can you add a preference please if it's enabled by default now?
(In reply to alexander :surkov from comment #63)
> (In reply to Trevor Saunders (:tbsaunde) from comment #62)
> > remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/33125ece9497
> 
> it'd be good if you were addressed comments before landing

you just said maybe, and I decided it wasn't worth the bother.  Besides its been there for a while already.

> > remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/06ea411856f7
> 
> can you add a preference please if it's enabled by default now?

I don't know what you want behind a pref or why, so why don't you do it since you care?
(In reply to Trevor Saunders (:tbsaunde) from comment #65)

> > can you add a preference please if it's enabled by default now?
> 
> I don't know what you want behind a pref or why, so why don't you do it
> since you care?

np
From the last few comments I take it this was fixed in 37. Someone please correct me if that's wrong.
status-firefox37: --- → fixed
tracking-firefox37: ? → +
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: