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

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Hsin-Yi Tsai (she/her) [:hsinyi]]

[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

Comment 1

[Hsin-Yi Tsai (she/her) [:hsinyi]]

[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.

Comment 2

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: part 1 - fix

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?

Comment 3

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: part 2 - test

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 :\ )

Comment 4

[Edgar Chen [:edgar]]

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

Comment 5

[Edgar Chen [:edgar]]

Comment on attachment 8483946 [details] [diff] [review]
part 2 - test

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

Looks good, thank you.

Comment 6

[Hsin-Yi Tsai (she/her) [:hsinyi]]

https://tbpl.mozilla.org/?tree=Try&rev=b579c1ea441d with comment 4 addressed. Thanks for the review!

Comment 7

[Hsin-Yi Tsai (she/her) [:hsinyi]]

   https://hg.mozilla.org/integration/b2g-inbound/rev/a1fcb7a15577
   https://hg.mozilla.org/integration/b2g-inbound/rev/d021a35ac833

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a1fcb7a15577
https://hg.mozilla.org/mozilla-central/rev/d021a35ac833

Comment 9

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

blocking per comment 1.

Comment 10

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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

Comment 11

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(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

Comment 12

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: [v2.1] part 1 - fix

Comment 13

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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

Comment 14

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: [v2.1] part 2 - test. approval-mozilla-aurora+ (carrying from [v2.1] part 1)

Comment 15

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/68c19178a60d
https://hg.mozilla.org/releases/mozilla-aurora/rev/30b8a67017bb