943460 - [B2G][MMS] Attachment does not appear in the message until hitting Send button

Status: VERIFIED FIXED

(Core :: Security, defect)

Description

[Natalya Kot [:nkot]]

Attachment: logcat

Description:
Image attachment does not appear in the message until hitting Send button. It happens with both - camera taken images and gallery ones. 

Repro Steps:
1) Updated Buri to BuildID: 20131126052050
2) Open Messages app
3) New message
4) Tap the Clip icon to add attachment
5) Select Camera or Gallery option
6) Add image
7) Observe the message

Actual:
 The image does not appear in the message (will appear after hitting Send button)

Expected:
 The image appears in the message

Environmental Variables:
Device: Buri v1.3 M-C Mozilla RIL
BuildID: 20131126052050
Gaia: 4ad796b196d468bdb231beba4392acbc90a74e96
Gecko: 99479edbee2a
Version: 28.0a1
Firmware Version: V1.2_20131115

Notes: does not reproduce on v1.2

Repro frequency: 100%
See attached screenshot and logcat

Comment 1

[Natalya Kot [:nkot]]

Attachment: screenshot

Comment 2

[Tony Nguyen]

This issue also occurs when attaching an audio file or video.

Comment 3

[Peter Bylenga [:PBylenga]]

Last build where issue did not reproduce:

Environmental Variables
Device: Buri v1.3 Mozilla RIL
Build ID: 20131125142741
Gecko: http://hg.mozilla.org/mozilla-central/rev/757c2011df5b
Gaia: bd8053d30c275f8d3040cd494e04b3480a784656
Platform Version: 28.0a1
RIL Version: 01.02.00.019.102 
Firmware Version: v1.2_20131115

First build where issue does reproduce:

Environmental Variables
Device: Buri v1.3 Mozilla RIL
Build ID: 20131126052050
Gecko: http://hg.mozilla.org/mozilla-central/rev/99479edbee2a
Gaia: 4ad796b196d468bdb231beba4392acbc90a74e96
Platform Version: 28.0a1
RIL Version: 01.02.00.019.102 
Firmware Version: v1.2_20131115

Comment 4

[Jason Smith [:jsmith]]

There's three commits within the regression range:

* https://github.com/mozilla-b2g/gaia/commit/31586bb4b49302c4ff43e9e937317c8038626e89
* https://github.com/mozilla-b2g/gaia/commit/53906820ec37e5ee8d16ddd7ad81ffc294407820
* https://github.com/mozilla-b2g/gaia/commit/2d8bed3ca415665f8d6e9183ac9b7c8ef8bf5d62

Julien - Which one broke this?

Comment 5

[Jason Smith [:jsmith]]

This bug feels more Gaia related than gecko related, but here's the push log for reference.

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=757c2011df5b&tochange=99479edbee2a

Comment 6

[Julien Wajsberg [:julienw]]

On a first glance none of these commits look suspicious.

2d8bed3ca415665f8d6e9183ac9b7c8ef8bf5d62 has been cherry-picked to 1.2 so it can be interesting to know if that happens in 1.2 too. This is the only commit among those 3 that could remotely break this, although I doubt it.

The image is displayed differently in the thread and in the composer. In the composer, it is a sandboxed iframe, so it's really possible that a Gecko change broke this.

Comment 7

[Joe Cheng [:jcheng] (please needinfo)]

traige: v1.3+ regression

Comment 8

[Julien Wajsberg [:julienw]]

In the SMS app, we add an iframe with "about:blank" and then register a load event to put a content inside the iframe.

So the problem is that I don't see the load event happening anymore.
Maybe Gecko does not dispatch a load event for this special page anymore?

Comment 9

[Julien Wajsberg [:julienw]]

I get the load event when reproducing that case in Firefox Nightly but I definitely don't get it on the device.

I'll try to run Gaia in Firefox and see.

Comment 10

[Julien Wajsberg [:julienw]]

I can't get an activity working in Firefox Nightly with the Browser extension right now so I can't try it.

Tried the reduced testcase on the device browser too and I get the load event.

So I'm quite stuck right now, I don't know the cause.

Comment 11

[Julien Wajsberg [:julienw]]

As an additional datapoint, I tried the SMS app from hash bd8053d30c275f8d3040cd494e04b3480a784656 (last working build) on a gecko from today => the issue is reproducing.

So this is definitely a gecko change. But this still may be a Gaia issue of course, assuming something where we should not.

Comment 12

[Julien Wajsberg [:julienw]]

Starting a bisect on Gecko, won't finish today.

Comment 13

[Julien Wajsberg [:julienw]]

Issue is caused by:

165005   91c18951d81a   2013-11-20 21:05 -0800   deian
  Bug 935690 - InitCSP checks SetCsp failure. r=bz

Comment 14

[Julien Wajsberg [:julienw]]

Hey Deian,

Your change in bug 935690 broke the display of attachments in in-progress messages in the SMS app. I'm sure this is more an issue in our app but I'd like to know what we're doing wrong.

The relevant code is [1] and [2].

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/js/attachment.js#L171-L193
[2] https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/js/compose.js#L173-L185

Thanks!

Comment 16

[Deian Stefan]

(In reply to Julien Wajsberg [:julienw] from comment #14)
> Your change in bug 935690 broke the display of attachments in in-progress
> messages in the SMS app. I'm sure this is more an issue in our app but I'd
> like to know what we're doing wrong.
> 
> The relevant code is [1] and [2].
> 
> [1]
> https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/js/attachment.
> js#L171-L193
> [2]
> https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/js/compose.js#L173-
> L185

I managed to reproduce the bug. The problem is that the app principal
(app://sms.gaiamobile.org) is set as the owner of the about:blank
document, so we hit the failure when we try to set CSP during the
iframe load. So, not a bug in your code!

This problem does not arise for the non-app case because we don't have
headers, so we return early
(https://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp#2621). In
the app case we try to set the default CSP (again).

From my (limited) understanding of B2G's use of CSP: the CSP for apps
should not change throughout the lifetime of the app. So, I think we
can safely return early if the document is an app and we've already
set CSP (i.e., it's not null). Sid: mind confirming this?

Comment 17

[Deian Stefan]

Note to self:

We should also cleanup the code a bit (e.g., https://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp#2724 is redundant since we assume early on that csp is not null)

Comment 18

[Julien Wajsberg [:julienw]]

I added the same CSP than certified apps in my testcase in http://everlong.org/mozilla/iframe-load/, so I don't get why I don't get the same issue ;)

Thanks anyway, I'm moving components then :)

Comment 19

[Julien Wajsberg [:julienw]]

Deian, any update here? It's really a big bug in Firefox OS SMS app.

If we should/can workaround the bug, please tell me what we can try to make it work correctly again.

Thanks!

Comment 20

[Deian Stefan]

(In reply to Julien Wajsberg [:julienw] from comment #19)
> Deian, any update here? It's really a big bug in Firefox OS SMS app.
> 
> If we should/can workaround the bug, please tell me what we can try to make
> it work correctly again.

Garrett confirmed my take on this (comment 16), I'll have a patch for this tonight/early tomorrow.

Comment 21

[Deian Stefan]

Attachment: 0001-Bug-943460-Apps-only-set-CSP-once.patch

Comment 22

[Deian Stefan]

Try is looking good: https://tbpl.mozilla.org/?tree=Try&rev=90ee5a3b2206

Comment 23

[Garrett Robinson [:grobinson]]

Comment on attachment 8344268 [details] [diff] [review]
0001-Bug-943460-Apps-only-set-CSP-once.patch

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

LGTM. You should add a comment explaining *why* we are returning early for apps if CSP is already set. Pinging Sid for review just to be safe.

Comment 24

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8344268 [details] [diff] [review]
0001-Bug-943460-Apps-only-set-CSP-once.patch

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

This patch will skip attaching a new CSP if the document already has one (as you intend it).  My concern with this approach is twofold:

1.  What if we end up wanting to refine the CSP during the app's lifetime? This will prohibit that.
2.  If the app's principal changes, then the semantics of 'self' change (from about:blank to app://whateverapp).

To address 1, you can put some debugging info into the bit of code that skips setting the CSP -- which you did.
To address 2, we need to make confirm the re-initialization doesn't mess with 'self'.  Looks like the iframe is inheriting the right principal and the right CSP, so this is less of a worry for this exact case.  The logging will help any future "modification of app CSP" bugs easier to track down.

Anyway, r=me if you tweak the debugging message so it's more useful if we have to debug multiply-set app CSPs in the future.

::: content/base/src/nsDocument.cpp
@@ +2652,5 @@
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    if (csp) {
> +#ifdef PR_LOGGING
> +      PR_LOG(gCspPRLog, PR_LOG_DEBUG, ("Document is an app for which CSP was already set."));

Maybe make this a little more specific explaining what the code is doing: "Document being inited already has a CSP, and it's an app, so we're going to skip re-setting the CSP" or something.

Comment 25

[Deian Stefan]

Thanks both for looking at this.

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #24)
> This patch will skip attaching a new CSP if the document already has one (as
> you intend it).  My concern with this approach is twofold:
> 
> 1.  What if we end up wanting to refine the CSP during the app's lifetime?
> This will prohibit that.

This actually doesn't affect refining the CSP. Since SetCsp fails on
all but the first set, we need to modify the policy with Append/RemovePolicy.

Comment 26

[Deian Stefan]

Attachment: 0001-Bug-943460-Apps-only-set-CSP-once.patch

Carrying over r+ from grobinson and sstamm

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/6c646d065358

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6c646d065358

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a343a740c4b6

Comment 31

[Loli (:lolimartinezcr)]

Tested
1.3 
12/12
Gaia cbd2921
Gecko c8ebb7f