Open Bug 680311 Opened 13 years ago Updated 2 years ago

Investigate android sync message queues for deadlock conditions


(GeckoView :: General, defect, P5)




Tracking Status
fennec + ---


(Reporter: dougt, Unassigned)



(Keywords: hang, mobile)


(1 file, 1 obsolete file)

In Bug #661978, we found that under certain conditions, we can deadlock the gecko thread.  We solve this by nesting an event loop to process incoming messages.

This bug is to investigate other sync messages queues we use.
Assignee: nobody → doug.turner
Attached patch patch (obsolete) — Splinter Review
Assignee: doug.turner → blassey.bugs
Attachment #555016 - Flags: review?(doug.turner)
brad and i discuss this a bit last week.  I don't like the idea of changing all of these call sites over to spinning a nested loop.  I filed the bug mostly to investigate other call sites for similar deadlocking conditions.  We should only spin a nested loop when we completely understand the events that trigger a deadlock.

He suggested that in cases we do not completely understand the event loop (every cases but the file picker dialog), the initial wait would be >= 1s.  Then we would spin a event loop.  If we did this, we should add logging to see how often this is hit and we should use that to investigate further.
Attachment #555016 - Flags: review?(doug.turner) → review-
Attached patch patchSplinter Review
this implements what dougt and I discussed. Essentially, behavior is unchanged unless we block for more than 1s, in which case we'll spin up the gecko message loop
Attachment #555016 - Attachment is obsolete: true
Attachment #562843 - Flags: review?(doug.turner)
Comment on attachment 562843 [details] [diff] [review]

so, for the file input case, we really don't want to wait 1 second first.
Attachment #562843 - Flags: review?(doug.turner) → review?(mark.finkle)
Comment on attachment 562843 [details] [diff] [review]

I am not tall enough to review this code
Attachment #562843 - Flags: review?(mark.finkle) → review?(doug.turner)
Comment on attachment 562843 [details] [diff] [review]

Review of attachment 562843 [details] [diff] [review]:

don't do that first 1s wait. always just loop for 1ms.  sorry for the long delay at reviewing this.
Attachment #562843 - Flags: review?(doug.turner) → review+
also, keep in mind, that I am not sure why we'd need to spin any loop other than for the filepicker.  in that case we know that we have reentrancy.  doing the rest, seems to be overkill, but if it makes flash better..
Is this patch/bug still useful for Native? I see the file still exists in hg and looks like the sections of code it modifies still exist.
yes, still relevant to native fennec
filter on [mass-p5]
Priority: -- → P5
Assignee: lassey → nobody

Moving all open Core::Widget: Android bugs to GeckoView::General (then the triage owner of GeckoView will decide which ones are valuable and which ones should be closed).

Component: Widget: Android → General
Product: Core → GeckoView
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.