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

RESOLVED FIXED

Status

Firefox OS
RIL
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: edgar, Assigned: edgar)

Tracking

unspecified
ARM
Gonk (Firefox OS)
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Updated

5 years ago
Depends on: 883178
(Assignee)

Comment 1

5 years ago
Created attachment 816520 [details] [diff] [review]
Fix the potential issue in MMI request handler, v1

Quick patch and I am trying to add a test case for the request error.
(Assignee)

Comment 2

5 years ago
Created attachment 816656 [details] [diff] [review]
Fix the potential issue in MMI request handler, v2

Add marionette test for error case.
Attachment #816520 - Attachment is obsolete: true
(Assignee)

Comment 3

5 years ago
Created attachment 816942 [details] [diff] [review]
Fix the potential issue in MMI request handler, v3
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)
(Assignee)

Comment 5

5 years ago
(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
(Assignee)

Comment 6

5 years ago
(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+
(Assignee)

Updated

5 years ago
Blocks: 926856
(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
(Assignee)

Comment 9

5 years ago
(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
(Assignee)

Comment 11

5 years ago
Created attachment 817097 [details] [diff] [review]
Fix the potential issue in MMI request handler, v4, r=allstars.chh

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
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.