Closed Bug 974054 Opened 6 years ago Closed 6 years ago

Compositor hangs when date goes back by years

Categories

(Core :: IPC, defect, P2)

ARM
Gonk (Firefox OS)
defect

Tracking

()

RESOLVED FIXED
mozilla30
blocking-b2g 1.3+
Tracking Status
firefox28 --- wontfix
firefox29 --- wontfix
firefox30 --- fixed
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed

People

(Reporter: tkundu, Assigned: bent.mozilla)

References

(Depends on 1 open bug)

Details

(Whiteboard: [caf priority: p1][CR 606898])

Attachments

(2 files, 1 obsolete file)

Compositor hangs if time goes backward.
blocking-b2g: --- → 1.3?
Component: General → Graphics: Layers
Product: Firefox OS → Core
This suggests that somebody isn't using a monotonic clock
tk has a WIP patch to the ipc/chromium code that will be shared soon.
Assignee: tkundu → nobody
blocking-b2g: 1.3? → 1.3+
Summary: Compositor hangs when date goes back to 1922 year → Compositor hangs when date goes back by years
(In reply to Dave Hylands [:dhylands] from comment #1)
> This suggests that somebody isn't using a monotonic clock

Yep!
This is a great find. I have seen a bug fly by re: compositor hanging. This absolutely makes sense. ++CA
Component: Graphics: Layers → IPC
:preeti -- Can you please find an owner of this bug who can help get the patch reviewed and carry this forward? We don't have expertise in this code.
Flags: needinfo?(praghunath)
This will need a more general fix. This can break other systems too. I am ok with landing this hack though while we do the real fix. bent, any opinions here?
(In reply to Andreas Gal :gal from comment #7)
> I am ok with landing this hack though while we do the real fix

We can maintain the hack over here for now, no need to clutter up the main commit log
Flags: needinfo?(praghunath)
FYI - If you do end up wanting a reviewer on this area of the code, then benjamin@smedbergs.us would be a good person to ask.

I am a bit confused though why this is being considered a 1.3 blocker. A lot of this code has been unchanged since early 2009 (i.e. we've likely shipped with this bug in every FxOS release). We also already have a bunch of other problems that happen if someone messes with the year on the phone - see bug 885864 that proposes a mitigation to ensure the user doesn't footgun themselves.

mvines - Why is this a CS blocker if we've been granted CS in every other FxOS release before with this issue?
Flags: needinfo?(mvines)
> mvines - Why is this a CS blocker if we've been granted CS in every other FxOS release before with this issue?

With more cores running around this issue is now easily reproducible.  eg, user moves backwards across a timezone and their device locks up for 1 hour while the ipc blocks.
Flags: needinfo?(mvines)
Sry, I should also note that we are *still* nowhere near the stability goals for v1.3.  This bug is one of the main reasons why.
The upstream (chromium) fix is here: https://code.google.com/p/chromium/issues/detail?id=293736

Any arguments against simply applying that fix here?

We can skip the NaCl fixes :)
bsmedberg, thoughts on comment 12?
Flags: needinfo?(benjamin)
Taking the change at https://code.google.com/p/chromium/issues/detail?id=293736 looks righteous enough. For desktop I'd like that to ride the trains, but I don't mind uplifting for B2G specially.
Flags: needinfo?(benjamin)
Tapas, could you try the attached patch and see if it still works?
Flags: needinfo?(tkundu)
Comment on attachment 8379133 [details] [diff] [review]
Chromium patch for FF-28 (B2G-1.3), v1

Hm, that didn't build. One sec.
Attachment #8379133 - Attachment is obsolete: true
Flags: needinfo?(tkundu)
Attached patch Patch, v1Splinter Review
Ok, try liked this one. I had to tweak the build_config.h flag to set OS_ANDROID appropriately.
Assignee: nobody → bent.mozilla
Status: NEW → ASSIGNED
Attachment #8379273 - Flags: review?(benjamin)
Tapas, can you try this patch and see if everything looks good?
Flags: needinfo?(tkundu)
(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #20)
> Tapas, can you try this patch and see if everything looks good?

This patch looks good to me. I am testing this patch internally. I will update soon here. Thanks
Comment on attachment 8379273 [details] [diff] [review]
Patch, v1

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

It looks fine.
Attachment #8379273 - Flags: review?(benjamin) → review+
It works as expected.
Flags: needinfo?(tkundu)
Comment on attachment 8379273 [details] [diff] [review]
Patch, v1

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

Thanks Tapas!

This still needs bsmedberg's review since I changed the build_config.h file.
Attachment #8379273 - Flags: review+ → review?(benjamin)
Attachment #8379273 - Flags: review?(benjamin) → review+
Comment on attachment 8379273 [details] [diff] [review]
Patch, v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): This bug has been around ever since we initially imported chromium code for IPC.
User impact if declined: Random hangs as described above.
Testing completed: :anded to m-i today, part of chromium since october 2013.
Risk to taking this patch (and alternatives if risky): This patch touches some very low level synchronization primitives and I would consider it very risky. It has existed in chromium upstream for a while now though so hopefully the risk is actually very small.
String or UUID changes made by this patch: None.
Attachment #8379273 - Flags: approval-mozilla-b2g28?
https://hg.mozilla.org/mozilla-central/rev/ca2cbc610c1b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #26)
> Comment on attachment 8379273 [details] [diff] [review]
> Patch, v1
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): This bug has been around ever
> since we initially imported chromium code for IPC.
> User impact if declined: Random hangs as described above.
> Testing completed: :anded to m-i today, part of chromium since october 2013.
> Risk to taking this patch (and alternatives if risky): This patch touches
> some very low level synchronization primitives and I would consider it very
> risky. It has existed in chromium upstream for a while now though so
> hopefully the risk is actually very small.

Given this, lets give a little bake-time on trunk/master before uplifting.
> String or UUID changes made by this patch: None.
Tapas - can you please land attachment 8379273 [details] [diff] [review] as a Gecko patch on CAF.  We'll back it out once the uplift occurs upstream.
Flags: needinfo?(tkundu)
(In reply to Michael Vines [:m1] [:evilmachines] from comment #29)
> Tapas - can you please land attachment 8379273 [details] [diff] [review] as
> a Gecko patch on CAF.  We'll back it out once the uplift occurs upstream.

done
Flags: needinfo?(tkundu)
Attachment #8379273 - Flags: approval-mozilla-b2g28? → approval-mozilla-b2g28+
Flags: in-moztrap?
Flags: in-moztrap? → in-moztrap-
Whiteboard: [CR 606898] → [caf priority: p1][CR 606898]
Depends on: 1062456
You need to log in before you can comment on or make changes to this bug.