740710 - B2G SMS: Timezone offset sign is wrong

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Tim Guan-tin Chien [:timdream] (please needinfo)]

When trying to send an SMS and send back one, the timestamp shown in log is hours into the future. Not a Gaia issue.

I/Gecko ( 2614): SmsDatabaseService: Going to store {"delivery":"sent","sender":null,"receiver":"[phone]","body":"Hello!","timestamp":1332999591630,"id":11} I/Gecko ( 2614): SmsDatabaseService: Going to store {"delivery":"received","sender":"[phone]","receiver":null,"body":"Hello back!","timestamp":1333057268000,"id":12}

It has been reported people on different timezone sees different offsets.

Gaia issue: https://github.com/andreasgal/gaia/issues/1074#issuecomment-4797291

Comment 1

[Philipp von Weitershausen [:philikon]]

It looks a lot like the timezone offset is added at least twice to the timestamp. I bet the RIL worker and/or the conversion of timestamp to Date object to timestamp to Date object is messing it up somewhere.

Comment 2

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Yeah, good catch. the received message is 16 hrs and 76.37 seconds ahead of the sent message. 76.37 secs is reasonable typing time of mine during the manual test.

Comment 3

[Kan-Ru Chen [:kanru] (UTC+9)]

I'd like to look into this.

Comment 4

[Philipp von Weitershausen [:philikon]]

Go for it! Thanks!

Comment 5

[Tim Guan-tin Chien [:timdream] (please needinfo)]

\O/

Comment 6

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: timezone offset sign is wrong

Need to rebase because bug 736697 touched here.

Comment 8

[Philipp von Weitershausen [:philikon]]

Comment on attachment 612843 [details] [diff] [review]
timezone offset sign is wrong

>       // If the most significant bit of the least significant nibble is 1,
>       // the timezone offset is negative (fourth bit from the right => 0x08).
>+      //   localtime = UTC + tzOffset
>+      // so
>+      //   UTC = localtime - tzOffset

s/so/therefore/

Yeah, good catch! Apparently I suck at math :)

r=me. I can address the small change above and push this fix. Thanks!

Comment 9

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Philipp von Weitershausen [:philikon] from comment #8)
> Comment on attachment 612843 [details] [diff] [review]
> timezone offset sign is wrong
> 
> >       // If the most significant bit of the least significant nibble is 1,
> >       // the timezone offset is negative (fourth bit from the right => 0x08).
> >+      //   localtime = UTC + tzOffset
> >+      // so
> >+      //   UTC = localtime - tzOffset
> 
> s/so/therefore/
> 
> Yeah, good catch! Apparently I suck at math :)
> 
> r=me. I can address the small change above and push this fix. Thanks!

Yes, push it! :)

Comment 10

[Philipp von Weitershausen [:philikon]]

(In reply to Kan-Ru Chen [:kanru] from comment #6)
> Need to rebase because bug 736697 touched here.

Next time, please do this asap right after asking for review. That way it creates less churn when pushing the patch. Thanks!

Comment 11

[Philipp von Weitershausen [:philikon]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d7a4dfac3acf

Comment 12

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Philipp von Weitershausen [:philikon] from comment #10)
> (In reply to Kan-Ru Chen [:kanru] from comment #6)
> > Need to rebase because bug 736697 touched here.
> 
> Next time, please do this asap right after asking for review. That way it
> creates less churn when pushing the patch. Thanks!

Ah.. I was meant to rebase when the changes hit m-c. Thanks for you super fast review :)

Comment 13

[Philipp von Weitershausen [:philikon]]

(In reply to Kan-Ru Chen [:kanru] from comment #12)
> (In reply to Philipp von Weitershausen [:philikon] from comment #10)
> > (In reply to Kan-Ru Chen [:kanru] from comment #6)
> > > Need to rebase because bug 736697 touched here.
> > 
> > Next time, please do this asap right after asking for review. That way it
> > creates less churn when pushing the patch. Thanks!
> 
> Ah.. I was meant to rebase when the changes hit m-c.

Oh I see. They just hit m-c from inbound. That's fair.

Comment 14

[Tim Guan-tin Chien [:timdream] (please needinfo)]

@kanru, you are the man!

Comment 15

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/d7a4dfac3acf