Compositor hangs if time goes backward.

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.

(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

patch attached

: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.

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

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?

> 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.

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?

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.

Here's the patch merged to the 1.3 codebase. https://tbpl.mozilla.org/?tree=Try&rev=47c7e1e0ceda

Tapas, could you try the attached patch and see if it still works?

Comment on attachment 8379133 [details] [diff] [review] Chromium patch for FF-28 (B2G-1.3), v1 Hm, that didn't build. One sec.

Silly merge mistake, new build going: https://tbpl.mozilla.org/?tree=Try&rev=7ed3126aff7e

Ok, try liked this one. I had to tweak the build_config.h flag to set OS_ANDROID appropriately.

Tapas, can you try this patch and see if everything looks good?

(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.

It works as expected.

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.

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.

(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.

(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