Closed Bug 1170153 Opened 9 years ago Closed 9 years ago

Nightly content process crash in mozilla::dom::PBrowserChild::SendPDocAccessibleConstructor

Categories

(Core :: Disability Access APIs, defect)

x86
Windows NT
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
e10s ? ---
firefox40 --- unaffected
firefox41 --- affected
firefox42 + fixed

People

(Reporter: kairo, Assigned: tbsaunde)

References

Details

(Keywords: crash, topcrash)

Crash Data

Attachments

(1 file)

[Tracking Requested - why for this release]:

This bug was filed from the Socorro interface and is 
report bp-ec768f07-ed31-40cd-875d-44cba2150601.
=============================================================

Top Frames:
0 	xul.dll 	mozilla::dom::PBrowserChild::SendPDocAccessibleConstructor(mozilla::a11y::PDocAccessibleChild*, mozilla::a11y::PDocAccessibleChild*, unsigned __int64 const&) 	obj-firefox/ipc/ipdl/PBrowserChild.cpp
1 	xul.dll 	mozilla::a11y::DocManager::CreateDocOrRootAccessible(nsIDocument*) 	accessible/base/DocManager.cpp
2 	xul.dll 	mozilla::a11y::DocManager::GetDocAccessible(nsIDocument*) 	accessible/base/DocManager.cpp
3 	xul.dll 	mozilla::a11y::DocManager::GetDocAccessible(nsIPresShell const*) 	accessible/base/DocManager.h
4 	xul.dll 	nsAccessibilityService::ContentRangeInserted(nsIPresShell*, nsIContent*, nsIContent*, nsIContent*) 	accessible/base/nsAccessibilityService.cpp
5 	xul.dll 	nsCSSFrameConstructor::ContentRangeInserted(nsIContent*, nsIContent*, nsIContent*, nsILayoutHistoryState*, bool) 	layout/base/nsCSSFrameConstructor.cpp
6 	xul.dll 	nsCSSFrameConstructor::ContentInserted(nsIContent*, nsIContent*, nsILayoutHistoryState*, bool) 	layout/base/nsCSSFrameConstructor.cpp
7 	xul.dll 	PresShell::Initialize(int, int) 	layout/base/nsPresShell.cpp
8 	xul.dll 	nsContentSink::StartLayout(bool) 	dom/base/nsContentSink.cpp
9 	xul.dll 	nsHtml5TreeOpExecutor::StartLayout() 	parser/html/nsHtml5TreeOpExecutor.cpp
10 	xul.dll 	nsHtml5TreeOperation::Perform(nsHtml5TreeOpExecutor*, nsIContent**) 	parser/html/nsHtml5TreeOperation.cpp
11 	xul.dll 	nsHtml5TreeOpExecutor::RunFlushLoop() 	parser/html/nsHtml5TreeOpExecutor.cpp
12 	xul.dll 	nsHtml5ExecutorFlusher::Run() 	parser/html/nsHtml5StreamParser.cpp
13 	xul.dll 	nsThread::ProcessNextEvent(bool, bool*) 	xpcom/threads/nsThread.cpp

This crash started with the 20150530030205 build on Nightly 41.0a1 and is currently the #2 crash on Nightly overall. It looks like all those crashes are in the content process and are EXCEPTION_ACCESS_VIOLATION_READ, at address 0x1a8 for 64bit builds or 0x170 for 32bit. They happen on all Windows versions from XP (like bp-46e8dc3f-3975-4ada-8357-89fd32150531) to Win10 (like bp-80c67d34-d1c8-40d8-942a-80f1f2150531).
This looks like fall-out from the second-to-last batch of e10s fixes from this merge push:
http://hg.mozilla.org/mozilla-central/rev/45a4d6336c73. For that build, bug 1168204, bug 1168202, bug 1167358, bug 1167604, and bug 1167295. That last one sounds like a likely candidate, but I'll leave it to tbsaunde to figure out the details. :)
Tracking for 41 because currently top crash.
Adding Trevor as an assignee because we want every tracked bug to be assigned.
Assignee: nobody → tbsaunde+mozbugs
We should never create DocAccessibles for documents that are going away so it
seems like this shouldn't be necessary, but without a test case its hard to
know why we are creating DocAccessibles for documents without docshells.  So
for now work around the issue and hope it doesn't matter in practice.
Attachment #8629935 - Flags: review?(dbolter)
Comment on attachment 8629935 [details] [diff] [review]
check documents have a docshell before trying to tell the parent process about new remote DocAccessibles

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

r=me with the inline comment addressed. The patch seems like a reasonable (good) thing to do in the circumstance.

::: accessible/base/DocManager.cpp
@@ +457,5 @@
>      if (IPCAccessibilityActive()) {
>        DocAccessibleChild* ipcDoc = new DocAccessibleChild(docAcc);
>        docAcc->SetIPCDoc(ipcDoc);
> +      nsIDocShell* docShell = aDocument->GetDocShell();
> +      if (aDocument) {

Check docShell here instead.
Attachment #8629935 - Flags: review?(dbolter) → review+
Top crasher on aurora.
tracking-e10s: --- → ?
Does ipdl glue auto translate from DocAccessibleChild* to DocAccessibleParent* for you?
(In reply to Jim Mathies [:jimm] from comment #7)
> Top crasher on aurora.

honestly I'm inclined to just disabled e10s with accessibility there at this point, just because I don't want to spend time thinking about it.


(In reply to Jim Mathies [:jimm] from comment #8)
> Does ipdl glue auto translate from DocAccessibleChild* to
> DocAccessibleParent* for you?

hrm? there just standard ipdl actor classes, what does that have to do with the bug anyway?
(In reply to Trevor Saunders (:tbsaunde) from comment #9)
> (In reply to Jim Mathies [:jimm] from comment #7)
> > Top crasher on aurora.
> 
> honestly I'm inclined to just disabled e10s with accessibility there at this
> point, just because I don't want to spend time thinking about it.

If we do that then a11y will remain off for the rest of the e10s rollout. That will put us in a bad situation - we'll have to continue supporting non-e10s for a11y and there will be nothing to motivate the a11y team in supporting e10s. I don't think we want to slip down that slope, we should be working toward getting a11y working with e10s.

> (In reply to Jim Mathies [:jimm] from comment #8)
> > Does ipdl glue auto translate from DocAccessibleChild* to
> > DocAccessibleParent* for you?
> 
> hrm? there just standard ipdl actor classes, what does that have to do with
> the bug anyway?

We were diagnosing the crash, trying to figure out what was going wrong. You pass a DocAccessibleChild* in on one end and on the other side you assume a DocAccessibleParent*. I was trying to figure out how that conversion takes place.
https://hg.mozilla.org/mozilla-central/rev/cf4897732a5f
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Created bug 1181177 since crash not fixed by this patch.
Jim, I know the patch for this one landed in moz-central yesterday. Should we uplift this to Aurora in 2-3 days after giving it enough bake time on trunk? Also, how difficult would it be to verify the fix? Is there a reason this is not tracking-e10s:+ ?
Flags: needinfo?(jmathies)
(In reply to Ritu Kothari (:ritu) from comment #13)
> Jim, I know the patch for this one landed in moz-central yesterday. Should
> we uplift this to Aurora in 2-3 days after giving it enough bake time on
> trunk?

Yes, might as well flag for approval now too.

> Also, how difficult would it be to verify the fix?

Watch crash stats, if this fixes the problem the 7-7 or newer builds shouldn't see the crash, or maybe see a reduced level.

https://crash-stats.mozilla.com/report/list?product=Firefox&range_value=7&range_unit=days&date=2015-07-06&signature=mozilla%3A%3Adom%3A%3APBrowserChild%3A%3ASendPDocAccessibleConstructor%28mozilla%3A%3Aa11y%3A%3APDocAccessibleChild*%2C+mozilla%3A%3Aa11y%3A%3APDocAccessibleChild*%2C+unsigned+__int64+const%26%29&version=Firefox%3A42.0a1#tab-graph

> Is there a reason
> this is not tracking-e10s:+ ?

It is marked ? implying we will triage it at our next triage session. So really it ios tracked at this point, we just haven't decided what we'll do with it.
Flags: needinfo?(jmathies)
Blocks: 1181177
This doesn't look fixed, should I reopen or refile?
(In reply to Jim Mathies [:jimm] from comment #15)
> This doesn't look fixed, should I reopen or refile?

Oops, I was looking at stats for 41.0a2. Do we need an uplift?
(In reply to Jim Mathies [:jimm] from comment #16)
> (In reply to Jim Mathies [:jimm] from comment #15)
> > This doesn't look fixed, should I reopen or refile?
> 
> Oops, I was looking at stats for 41.0a2. Do we need an uplift?

I was right at first too, this is on 42.0a1 as well and shows up after the landing.
Ah, sorry I've totally confused this with bug 1181177.
Blocks: 1182472
(In reply to Jim Mathies [:jimm] from comment #17)
> (In reply to Jim Mathies [:jimm] from comment #16)
> > (In reply to Jim Mathies [:jimm] from comment #15)
> > > This doesn't look fixed, should I reopen or refile?
> > 
> > Oops, I was looking at stats for 41.0a2. Do we need an uplift?
> 
> I was right at first too, this is on 42.0a1 as well and shows up after the
> landing.

Should I re-open this bug because it shows up as fixed in trunk and begs the uplift-to-aurora question (again)?
Flags: needinfo?(jmathies)
(In reply to Ritu Kothari (:ritu) from comment #19)
> (In reply to Jim Mathies [:jimm] from comment #17)
> > (In reply to Jim Mathies [:jimm] from comment #16)
> > > (In reply to Jim Mathies [:jimm] from comment #15)
> > > > This doesn't look fixed, should I reopen or refile?
> > > 
> > > Oops, I was looking at stats for 41.0a2. Do we need an uplift?
> > 
> > I was right at first too, this is on 42.0a1 as well and shows up after the
> > landing.
> 
> Should I re-open this bug because it shows up as fixed in trunk and begs the
> uplift-to-aurora question (again)?

I'm not sure if this bug fixed that crash. We also have work in bug 1181177 which might help. That said the a11y team was planning on killing a11y on aurora with e10s in bug 1182097, but that never uplifted.

David, what are our plans here with 41?
Flags: needinfo?(jmathies) → needinfo?(dbolter)
(In reply to Jim Mathies [:jimm] from comment #20)
> (In reply to Ritu Kothari (:ritu) from comment #19)
> > (In reply to Jim Mathies [:jimm] from comment #17)
> > > (In reply to Jim Mathies [:jimm] from comment #16)
> > > > (In reply to Jim Mathies [:jimm] from comment #15)
> > > > > This doesn't look fixed, should I reopen or refile?
> > > > 
> > > > Oops, I was looking at stats for 41.0a2. Do we need an uplift?
> > > 
> > > I was right at first too, this is on 42.0a1 as well and shows up after the
> > > landing.
> > 
> > Should I re-open this bug because it shows up as fixed in trunk and begs the
> > uplift-to-aurora question (again)?
> 
> I'm not sure if this bug fixed that crash. We also have work in bug 1181177
> which might help. That said the a11y team was planning on killing a11y on
> aurora with e10s in bug 1182097, but that never uplifted.
> 
> David, what are our plans here with 41?

bug 1182097 just wasn't getting uplifted because bug queries missed which is apparently fixed now.
Flags: needinfo?(dbolter)
Jim, Trevor, is this issue fixed in FF41? I am trying to see if a patch needs to be uplifted to Beta41 or can we mark status-ff41 to fixed given that bug 1182097 is fixed now.
Flags: needinfo?(tbsaunde+mozbugs)
Flags: needinfo?(jmathies)
(In reply to Ritu Kothari (:ritu) from comment #22)
> Jim, Trevor, is this issue fixed in FF41? I am trying to see if a patch
> needs to be uplifted to Beta41 or can we mark status-ff41 to fixed given
> that bug 1182097 is fixed now.

it should be uneffected because e10s is off, but checking crash stats can't hurt.
Flags: needinfo?(tbsaunde+mozbugs)
yep, no e10s on 41 so hopefully this doesn't show up.
Flags: needinfo?(jmathies)
Untracked for FF41 as e10s is not enabled by default. Adding a tracking flag for FF42 instead.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: