Closed Bug 926358 Opened 6 years ago Closed 6 years ago

B2G RIL: If there are two sendMMI requests come at the same time, former one may not be handled correctly.

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: edgar, Assigned: edgar)

References

Details

Attachments

(1 file, 3 obsolete files)

In RILContentHelper, we use only one |_window| to cache the requester's window in sendMMI() [1]. But actually there is a potential issue here, if there are two requests come at the same time, the |_window| will be override by latter one. We should use |_windowsMap| to cache window, just like what we did in CardLock related function [2].

[1] http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/RILContentHelper.js#857
[2] Please see bug 873380.
Depends on: 883178
Quick patch and I am trying to add a test case for the request error.
Add marionette test for error case.
Attachment #816520 - Attachment is obsolete: true
Attachment #816656 - Attachment is obsolete: true
Attachment #816942 - Flags: review?(allstars.chh)
Comment on attachment 816942 [details] [diff] [review]
Fix the potential issue in MMI request handler, v3

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

This patch looks good to me.
But I'd like to add another test case for calling two sendMMI in a row to address what you have fixed in this patch.

::: dom/network/tests/marionette/test_mobile_mmi.js
@@ +76,4 @@
>      tasks.next();
>    }
>    request.onerror = function onerror() {
>      ok(false, "request success");

The message here is incorrect
Attachment #816942 - Flags: review?(allstars.chh)
(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #4)
> Comment on attachment 816942 [details] [diff] [review]
> Fix the potential issue in MMI request handler, v3
> 
> Review of attachment 816942 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This patch looks good to me.
> But I'd like to add another test case for calling two sendMMI in a row to
> address what you have fixed in this patch.
Sure, I will add another test case in next version, thank you.

> 
> ::: dom/network/tests/marionette/test_mobile_mmi.js
> @@ +76,4 @@
> >      tasks.next();
> >    }
> >    request.onerror = function onerror() {
> >      ok(false, "request success");
> 
> The message here is incorrect
(In reply to Edgar Chen [:edgar][:echen] from comment #5)
> (In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #4)
> > Comment on attachment 816942 [details] [diff] [review]
> > Fix the potential issue in MMI request handler, v3
> > 
> > Review of attachment 816942 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > This patch looks good to me.
> > But I'd like to add another test case for calling two sendMMI in a row to
> > address what you have fixed in this patch.
> Sure, I will add another test case in next version, thank you.

Hmm, after did some tests, even we calling two sendMMI in a row in marionette, the request still can be processed correctly. Because these two requests are belonging to the same window. I will file another bug for figuring out how to add a test case for this fix. Let's land this fix first.
Comment on attachment 816942 [details] [diff] [review]
Fix the potential issue in MMI request handler, v3

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

Add r=me
Attachment #816942 - Flags: review+
(In reply to Edgar Chen [:edgar][:echen] from comment #6)
> (In reply to Edgar Chen [:edgar][:echen] from comment #5)
> > (In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #4)
> > > Comment on attachment 816942 [details] [diff] [review]
> > > Fix the potential issue in MMI request handler, v3
> > > 
> > > Review of attachment 816942 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > This patch looks good to me.
> > > But I'd like to add another test case for calling two sendMMI in a row to
> > > address what you have fixed in this patch.
> > Sure, I will add another test case in next version, thank you.
> 
> Hmm, after did some tests, even we calling two sendMMI in a row in
> marionette, the request still can be processed correctly. Because these two
> requests are belonging to the same window. I will file another bug for
> figuring out how to add a test case for this fix. Let's land this fix first.

Not very sure, but seems [1] might help?! 

[1] http://mxr.mozilla.org/mozilla-central/source/dom/telephony/test/marionette/test_outgoing_emergency_in_airplane_mode.js#14
(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #8)
> (In reply to Edgar Chen [:edgar][:echen] from comment #6)
> > (In reply to Edgar Chen [:edgar][:echen] from comment #5)
> > > (In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #4)
> > > > Comment on attachment 816942 [details] [diff] [review]
> > > > Fix the potential issue in MMI request handler, v3
> > > > 
> > > > Review of attachment 816942 [details] [diff] [review]:
> > > > -----------------------------------------------------------------
> > > > 
> > > > This patch looks good to me.
> > > > But I'd like to add another test case for calling two sendMMI in a row to
> > > > address what you have fixed in this patch.
> > > Sure, I will add another test case in next version, thank you.
> > 
> > Hmm, after did some tests, even we calling two sendMMI in a row in
> > marionette, the request still can be processed correctly. Because these two
> > requests are belonging to the same window. I will file another bug for
> > figuring out how to add a test case for this fix. Let's land this fix first.
> 
> Not very sure, but seems [1] might help?! 
> 
> [1]
> http://mxr.mozilla.org/mozilla-central/source/dom/telephony/test/marionette/
> test_outgoing_emergency_in_airplane_mode.js#14

I have tried this way, but still doesn't work. :(
Thanks anyway.
(In reply to Edgar Chen [:edgar][:echen] from comment #9)
> (In reply to Hsin-Yi Tsai  [:hsinyi] from comment #8)
> > (In reply to Edgar Chen [:edgar][:echen] from comment #6)
> > > (In reply to Edgar Chen [:edgar][:echen] from comment #5)
> > > > (In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #4)
> > > > > Comment on attachment 816942 [details] [diff] [review]
> > > > > Fix the potential issue in MMI request handler, v3
> > > > > 
> > > > > Review of attachment 816942 [details] [diff] [review]:
> > > > > -----------------------------------------------------------------
> > > > > 
> > > > > This patch looks good to me.
> > > > > But I'd like to add another test case for calling two sendMMI in a row to
> > > > > address what you have fixed in this patch.
> > > > Sure, I will add another test case in next version, thank you.
> > > 
> > > Hmm, after did some tests, even we calling two sendMMI in a row in
> > > marionette, the request still can be processed correctly. Because these two
> > > requests are belonging to the same window. I will file another bug for
> > > figuring out how to add a test case for this fix. Let's land this fix first.
> > 
> > Not very sure, but seems [1] might help?! 
> > 
> > [1]
> > http://mxr.mozilla.org/mozilla-central/source/dom/telephony/test/marionette/
> > test_outgoing_emergency_in_airplane_mode.js#14
> 
> I have tried this way, but still doesn't work. :(
> Thanks anyway.

So sad, QQ
1). Correct the message mention in comment #4.
2). Add r=allstars.chh after r+.
Attachment #816942 - Attachment is obsolete: true
Attachment #817097 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/44a0355a4ead
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.