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

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

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

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?

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

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.

traige: v1.3+ regression

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?

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.

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.

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.

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

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

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!

(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?

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)

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

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!

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

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

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

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.

Carrying over r+ from grobinson and sstamm

Tested 1.3 12/12 Gaia cbd2921 Gecko c8ebb7f