Closed Bug 1054004 Opened 6 years ago Closed 6 years ago

[Messages] There is a possible race condition between navigator.mozL10n.ready and "DOMContentLoaded"

Categories

(Firefox OS Graveyard :: Gaia::SMS, defect, major)

ARM
Gonk (Firefox OS)
defect
Not set
major

Tracking

(blocking-b2g:2.0+, b2g-v2.0 fixed, b2g-v2.1 fixed)

RESOLVED FIXED
2.1 S3 (29aug)
blocking-b2g 2.0+
Tracking Status
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: azasypkin, Assigned: azasypkin)

References

Details

(Whiteboard: [sms-sprint-2.1S3])

Attachments

(4 files)

*** Follow-up from bug 1051852 ***

See comment 17 bug 1051852 for details.
[Blocking Requested - why for this release]: fixing a possible race condition that's made more visible with bug 1051852
blocking-b2g: --- → 2.0?
Hey Julien,

(In reply to Julien Wajsberg [:julienw] from comment #17)
> 2 possible solutions IMO:
> 
> * the other solution is, in appendThread, add at the start:
> 
>   if (navigator.mozL10n.readyState !== 'complete') {
>     navigator.mozL10n.once(this.appendThread.bind(this, thread));
>     return;
>   }
> 

I think this solution is the most appropriate here since "appendThread" is also called while rendering drafts that can causes such problems too.

> 
> Last solution is to avoid needing the l10n lib at this time (we need it
> because of Utils' date/time utility functions), because I _think_ we handle
> these items in the mozL10n's ready callback in startup.js anyway.

I tried, but it didn't seem to easy, we use for both timestamps and sticky header (Today, Yesterday and so on).

So this branch with the solution you suggested. I also experimented with more complex way that fires performance events ('moz-app-visually-complete' and 'moz-app-loaded') with respect to drafts and mozL10n readiness. I'll attach POC branch too.
Attachment #8474397 - Flags: feedback?(felash)
Attached file GitHub brach URL (POC)
Here is the POC I mentoned in comment 2
Attachment #8474398 - Flags: feedback?(felash)
I sat down with Steve to understand this better.
Potential user impact is that user might see non-localized strings. I'm setting it blocking for this.
blocking-b2g: 2.0? → 2.0+
(In reply to Wesley Huang [:wesley_huang] from comment #4)
> I sat down with Steve to understand this better.
> Potential user impact is that user might see non-localized strings. I'm
> setting it blocking for this.

Potential is a lot worse than this: the user could see a blank screen and the only way out would be to restart the application.
(In reply to Oleg Zasypkin [:azasypkin] from comment #2)
> > 
> > Last solution is to avoid needing the l10n lib at this time (we need it
> > because of Utils' date/time utility functions), because I _think_ we handle
> > these items in the mozL10n's ready callback in startup.js anyway.
> 
> I tried, but it didn't seem to easy, we use for both timestamps and sticky
> header (Today, Yesterday and so on).

The idea was to output the date in one format (say: the format that US doesn't use :) ) if the lib is not ready; then when the lib is ready it would change them thanks to the listener we set in startup.js.

The underlying goal is also to be able to render threads without waiting for the l10n lib, which can be good for the startup performance :)

> So this branch with the solution you suggested. I also experimented with
> more complex way that fires performance events ('moz-app-visually-complete'
> and 'moz-app-loaded') with respect to drafts and mozL10n readiness. I'll
> attach POC branch too.

Thanks, I'll look at it.
Comment on attachment 8474397 [details]
GitHub brach URL (easiest solution)

Just wondering if we should not make `readyState = 'complete'` the default inside MockL10n.
Attachment #8474397 - Flags: feedback?(felash) → feedback+
Comment on attachment 8474398 [details]
GitHub brach URL (POC)

Note: we can't easily comment on a "compare" view. Please either do a PR or attach a patch on the bug instead.

Some comments:
* startup.js: you're leaking the listeners here. Maybe it would be a good idea to have a "once" function in event_dispatcher.js that would take care of removing the handlers automatically.
* onThreadRendered: I don't get why it's called in render's promise callback
* I'm not very fond of the added complexity around renderThread.
* I like the fact that you replace the callbacks with 2 events.


As said earlier, as a longer term goal I'd like that we can add a thread even if l10n is not ready. IIRC The previous l10n library was not throwing in that case, and was just returning an empty string.
Maybe we should rewrite Utils.getFormattedHour and friends like the l10n lib was:
* only add data attributes
* register mutation observers to take care of them
* in the mutation observer, when mozL10n is not ready, bail out
* register to the "ready" event to react to locale changes (as we already do in startup.js)

That sounds like a better plan that adding complexity like this. What do you think?

For 2.0 let's stick with the previous simpler patch though, and let's file a separate bug to do all this.
Attachment #8474398 - Flags: feedback?(felash) → feedback-
(In reply to Julien Wajsberg [:julienw] (PTO 08/20 -> 09/15; contact schung instead) from comment #8)
> Comment on attachment 8474398 [details]
> GitHub brach URL (POC)
> 
> Note: we can't easily comment on a "compare" view. Please either do a PR or
> attach a patch on the bug instead.
> 
> Some comments:
> * startup.js: you're leaking the listeners here. Maybe it would be a good
> idea to have a "once" function in event_dispatcher.js that would take care
> of removing the handlers automatically.
> * onThreadRendered: I don't get why it's called in render's promise callback
> * I'm not very fond of the added complexity around renderThread.
> * I like the fact that you replace the callbacks with 2 events.
> 
> 
> As said earlier, as a longer term goal I'd like that we can add a thread
> even if l10n is not ready. IIRC The previous l10n library was not throwing
> in that case, and was just returning an empty string.
> Maybe we should rewrite Utils.getFormattedHour and friends like the l10n lib
> was:
> * only add data attributes
> * register mutation observers to take care of them
> * in the mutation observer, when mozL10n is not ready, bail out
> * register to the "ready" event to react to locale changes (as we already do
> in startup.js)
> 
> That sounds like a better plan that adding complexity like this. What do you
> think?

Yeah, as we were talking recently we expect l10n lib to be fast enough, so we don't need to build any complex workaround logic. The only thing I'm concerned about that "Utils.getFormattedHour" isn't the only place that throws exceptions in case l10n isn't ready. I've noticed that there is a possible race condition when l10n lib registered MutationObserver itself, but it was not ready yet, and at the same moment we were appending node with data-l10n attributes as result l10n failed in MutationObserver callback. Having said that, we may need some fixes in l10n lib itself if it breaks localization somehow.

> For 2.0 let's stick with the previous simpler patch though, and let's file a
> separate bug to do all this.
Agree, the only thing that I'd like to borrow from this POC is the part that respects rendering of both threads and drafts before firing appropriate performance events.
> I've noticed that there is a possible race condition when l10n lib registered 
> MutationObserver itself, but it was not ready yet, and at the same moment we were appending 
> node with data-l10n attributes as result l10n failed in MutationObserver callback.

I filed a bug for this :) bug 1051850

> The only thing I'm concerned about that "Utils.getFormattedHour" isn't the only place that 
> throws exceptions in case l10n isn't ready

We should strive to fix all of them in the same way !
Oleg, I know it's not in the sprint, but we need to fix this quickly to let bug  	1051852 be uplifted as well. When this bug lands, don't forget to remove the NO_UPLIFT flag in the whiteboard in that bug too.

Thanks !
Comment on attachment 8474397 [details]
GitHub brach URL (easiest solution)

Hey Steve,

Could you please take a look at this patch?

Thanks!
Attachment #8474397 - Flags: review?(schung)
Attachment #8474397 - Flags: review?(schung)
Attached correct pull request
Attachment #8475990 - Flags: review?(schung)
Comment on attachment 8475990 [details] [review]
GitHub pull request URL

r=me since this patch is simple enough and unlikely to lead any additional regression(if mozl10n api works correctly), thanks!
Attachment #8475990 - Flags: review?(schung) → review+
(In reply to Steve Chung [:steveck](PTO 8/22) from comment #15)
> Comment on attachment 8475990 [details] [review]
> GitHub pull request URL
> 
> r=me since this patch is simple enough and unlikely to lead any additional
> regression(if mozl10n api works correctly), thanks!

And please note that we have to uplift 1051852 as well once this patch uplifted.
(In reply to Steve Chung [:steveck](PTO 8/22) from comment #16)
> (In reply to Steve Chung [:steveck](PTO 8/22) from comment #15)
> > Comment on attachment 8475990 [details] [review]
> > GitHub pull request URL
> > 
> > r=me since this patch is simple enough and unlikely to lead any additional
> > regression(if mozl10n api works correctly), thanks!
> 
> And please note that we have to uplift 1051852 as well once this patch
> uplifted.

I'm attaching separate v2.0 patch since master one can't be applied directly due to conflicts in unit tests. v2.0 differs only by reduced unit tests changes, so carrying on r+ here too.

Waiting for green Try.
Attachment #8477268 - Flags: review+
Master: https://github.com/mozilla-b2g/gaia/commit/de6d327b2c2854f1fcedd87dfca9cbbec3e63c43

Note to sheriffs: please uplift attachment 8477268 [details] [review] (GitHub pull request URL (v2.0)) to v2.0 branch.

Thanks!
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [sms-sprint-2.1S3]
You need to log in before you can comment on or make changes to this bug.