Closed Bug 1061535 Opened 6 years ago Closed 5 years ago

[B2G][RIL] relax restrictions on EF_IMG error handling

Categories

(Firefox OS Graveyard :: RIL, defect)

x86_64
Linux
defect
Not set

Tracking

(blocking-b2g:2.1+, firefox33 wontfix, firefox34 fixed, firefox35 fixed, b2g-v2.1 fixed, b2g-v2.2 fixed)

RESOLVED FIXED
2.1 S4 (12sep)
blocking-b2g 2.1+
Tracking Status
firefox33 --- wontfix
firefox34 --- fixed
firefox35 --- fixed
b2g-v2.1 --- fixed
b2g-v2.2 --- fixed

People

(Reporter: hsinyi, Assigned: hsinyi)

References

Details

Attachments

(4 files)

[1] has a strict check of data length per the spec. But we learned that in some real cases, paddings might be appended. We should relax the restriction a little bit, i.e.  if (octetLen < (9 * numInstances + 1)).

[1] http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js?from=ril_worker.js#13592
[Blocking Requested - why for this release]: it's a follow-up of feature-b2g 2.1: bug 935843. Without this, some STK icons may not be delivered correctly.
blocking-b2g: --- → 2.1?
Depends on: 935843
Attached patch part 1 - fixSplinter Review
Hi Edgar,

As I explained in comment 0, considering some real cases, we'd rather loosen the error criteria.

I also learned that, we should read all remaining data otherwise padding error is met in xpcshell tests.

Could you please take a look, thank you?
Attachment #8483943 - Flags: review?(echen)
Attached patch part 2 - testSplinter Review
Summary of the patch:
1) xpcshell-test: cover scenario of patch 1
2) marionette: error that should have been caught in bug 824145 (the test code path wasn't really run because of the too strict error handling :\ )
Attachment #8483946 - Flags: review?(echen)
Comment on attachment 8483943 [details] [diff] [review]
part 1 - fix

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

r=me with the below addressed.

Thank you.

::: dom/system/gonk/ril_worker.js
@@ +13626,5 @@
>          if (onerror) {
>            onerror();
>          }
> +        Buf.seekIncoming((octetLen - 1) * Buf.PDU_HEX_OCTET_SIZE);
> +        Buf.readStringDelimiter(strLen);

Seek and read string delimiter before trigger onerror callback.

@@ +13713,1 @@
>          return;

ditto
Attachment #8483943 - Flags: review?(echen) → review+
Comment on attachment 8483946 [details] [diff] [review]
part 2 - test

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

Looks good, thank you.
Attachment #8483946 - Flags: review?(echen) → review+
https://tbpl.mozilla.org/?tree=Try&rev=b579c1ea441d with comment 4 addressed. Thanks for the review!
https://hg.mozilla.org/mozilla-central/rev/a1fcb7a15577
https://hg.mozilla.org/mozilla-central/rev/d021a35ac833
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S4 (12sep)
blocking per comment 1.
blocking-b2g: 2.1? → 2.1+
Comment on attachment 8483943 [details] [diff] [review]
part 1 - fix

Approval Request Comment
[Feature/regressing bug #]: a flaw in feature-b2g 2.1: bug 935843
[User impact if declined]: for some sim cards, user may not be able to STK icons on UI
[Describe test coverage new/current, TBPL]: landed on m-c, new test case added for this case
[Risks and why]: low risk as this is a simple change and this is verified by tests
[String/UUID change made/needed]: no
Attachment #8483943 - Flags: approval-mozilla-aurora?
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #10)
> Comment on attachment 8483943 [details] [diff] [review]
> part 1 - fix
> 
> Approval Request Comment
> [Feature/regressing bug #]: a flaw in feature-b2g 2.1: bug 935843
> [User impact if declined]: for some sim cards, user may not be able to STK
> icons on UI

for some sim cards, user may not be able to *see* STK icons on UI

> [Describe test coverage new/current, TBPL]: landed on m-c, new test case
> added for this case
> [Risks and why]: low risk as this is a simple change and this is verified by
> tests
> [String/UUID change made/needed]: no
Attachment #8483943 - Flags: approval-mozilla-aurora?
Comment on attachment 8489244 [details] [diff] [review]
[v2.1] part 1 - fix

Approval Request Comment
[Feature/regressing bug #]: a flaw in feature-b2g 2.1: bug 935843
[User impact if declined]: for some sim cards, user may not be able to see STK icons on UI
[Describe test coverage new/current, TBPL]: landed on m-c, new test case added for this case
[Risks and why]: low risk as this is a simple change and this is verified by tests
[String/UUID change made/needed]: no
Attachment #8489244 - Flags: approval-mozilla-aurora?
Attachment #8489244 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8489245 [details] [diff] [review]
[v2.1] part 2 - test. approval-mozilla-aurora+ (carrying from [v2.1] part 1)

This part is for test cases, so I am simply carrying approval-mozilla-aurora+ from [v2.1] part 1.
Attachment #8489245 - Attachment description: [v2.1] part 2 - test → [v2.1] part 2 - test. approval-mozilla-aurora+ (carrying from [v2.1] part 1)
You need to log in before you can comment on or make changes to this bug.