1194525 - Gecko should ignore |postResult| calls for WebActivities with no returnValue

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P3)

Description

[Dietrich Ayala (:dietrich)]

User-Agent: Mozilla/5.0 (Mobile; rv:43.0) Gecko/43.0 Firefox/43.0

Twitter shows up and then immediately disappears, putting me back in gallery.

Comment 1

[Shally Li（leave from Mozilla）]

Hi Mahendra,

    When sharing the image from Gallery to Twitter on the latest Flame KK v2.2&2.5 and Aries KK v2.5, the Twitter app shows up and doesn't disappear, but it stays in the twitter app and doesn't switch to a new tweet, user can't share the image that user selected from Gallery, unless user create a new tweet and select the image from Gallery again. This problem are different from the above mentioned in comment 0. 
    Could you help to check this problem again? 

Thank you very much.


STR:
1.Sign in Twitter account (On Flame v2.2&2.5, you need to download the Twitter app).
2.Open Gallery app and select an image.
3.Tap "Share" icon and select "Twitter".
Actual results: The Twitter app shows up and doesn't disappear, but it stays in the twitter app and doesn't switch to a new tweet, then user can't share the image. 

Please see attachments: AriesKK_v2.5.3gp and logcat_1142.txt
Reproduce rate: 10/10


------------------------------------------------------------------------------------------
Device: Flame KK 2.2 (Affected) 
Build ID               20150817032503
Gaia Revision          335cd8e79c20f8d8e93a6efc9b97cc0ec17b5a46
Gaia Date              2015-08-14 19:06:41
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/82ec88fa015c
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150817.065716
Firmware Date          Mon Aug 17 06:57:27 EDT 2015
Firmware Version       v18D v4
Bootloader             L1TC000118D0

Device: Flame KK 2.5 (Affected)
Build ID               20150817150206
Gaia Revision          60489c1ff8c5d1633fc4837d4f8019623d4e1940
Gaia Date              2015-08-16 02:21:48
Gecko Revision         https://hg.mozilla.org/mozilla-
central/rev/9673c75864beafca2f6c8b117b98503128bf2e56
Gecko Version          43.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150817.182933
Firmware Date          Mon Aug 17 18:29:45 EDT 2015
Firmware Version       v18D v4
Bootloader             L1TC000118D0

Device: Aries KK 2.5(Affected)
Build ID               20150818005621
Gaia Revision          60489c1ff8c5d1633fc4837d4f8019623d4e1940
Gaia Date              2015-08-16 02:21:48
Gecko Revision         https://hg.mozilla.org/mozilla-
central/rev/6ae3e9ff53b2bae8d95a90c9f25368fd81fa357e
Gecko Version          43.0a1
Device Name            aries
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.worker.20150818.003312
Firmware Date          Tue Aug 18 00:33:21 UTC 2015
Bootloader             s1

Comment 2

[Shally Li（leave from Mozilla）]

Attachment: logcat_1142.txt

Comment 3

[Shally Li（leave from Mozilla）]

Attachment: AriesKK_v2.5.3gp

Comment 4

[Mahendra Potharaju [:mahe]]

Thanks for the verification and STR Shally. Will ask someone from Gallery team to take a look and find a right owner.

Comment 5

[No-Jun Park [:njpark]]

I don't think it's the gallery app's problem per se, but could be related to the pick activity.  I remember we had a similar issue with the twitter share a while ago.  Ni?ing djf in case he can remember what kind of issue it was, and which component we can direct this bug to.

Comment 6

[David Flanagan [:djf]]

No-Jun: I don't remember any similar bug with Twitter. We've got the same share activity bug with the Facebook app.  And we've had OOM-related bugs for activities involving photos before, but this one is not ringing any bells.

No one seems to be saying that this is a regression, are they? If this has ever worked in the past, then let me know and I'll investigate. Similarly, if you can share a photo directly from the camera to Twitter, then that would indicate that it is my bug in Gallery to fix.

Please also check whether the share activity works for small images (not photos from the camera). If so, then maybe there is a problem in our platform.

But if it has never worked, then this sounds like it is a Tech Evangelism bug, like the Facebook one. We should ask Twitter to either make the share activity work, or modify their manifest so that it does not advertise the ability to accept shared images.

Comment 7

[No-Jun Park [:njpark]]

I think the feature has worked in the past at some point: the resolution of bug 823762 seems to say the feature was added and verified.  I also found the bug I was pondering about, which is bug 983544 (still open for some reason)   

So... I think based on those, this is a regression bug.


Adding qawanted to test this with a small image (let's say, a 128 by 128 image)

Comment 8

[Dietrich Ayala (:dietrich)]

Yes, absolutely a regression. I used this regularly and it worked fine. That doesn't rule out it being a tech evangelism bug though. Twitter's share activity disappeared altogether for a few days last week.

Or it could be something similar to bug 1192703.

Comment 9

[Pi Wei Cheng [:piwei] (inactive)]

I think people are talking about different bugs here. The issue at comment 0 looks like bug 1192703, and the issue at comment 1 I recall it being a very old bug and has never been fixed (if it was fixed at some point then I didn't know about) - example bug 1003301.

Comment 10

[Dietrich Ayala (:dietrich)]

Correct, the scenario in comment #1 is not what I reported for this bug.

Bug 1192703, also reported by me, is now fixed. But it did not fix this bug. I mentioned it because it seems similar.

Comment 11

[Max Ivanov[:MaxIvanov]]

I can reproduce this on the Aires 2.5 and Flame 2.5. I've tested that also with a small image (128x128 px) and got the same result. Twitter opens and then immediately closes, user returns back to the gallery.Please check the video https://youtu.be/jo-ByfCmo1M

Environmental Variables:
Device: Aries 2.5
Build ID: 20150812231434
Gaia: 52f3ea58df38e5427f6afeb636bc6ad01d24022f
Gecko: 7649ffe28b67aa2dad0f67ea01500c0ff91b2bac
Gonk: 2916e2368074b5383c80bf5a0fba3fc83ba310bd
Version: 43.0a1 (2.5)
Firmware Version: D5803_23.1.A.1.28_NCB.ftf
User Agent: Mozilla/5.0 (Mobile; rv:43.0) Gecko/43.0 Firefox/43.0

Device: Flame 2.5
Build ID: 20150903041524
Gaia: a55d3d512a765bd483bd595b0c8f80c5f1d61b65
Gecko: 74fbd245369c474beaa7f2b1959570243e3dafaa
Gonk: c4779d6da0f85894b1f78f0351b43f2949e8decd
Version: 43.0a1 (2.5)
Firmware Version: v18D
User Agent: Mozilla/5.0 (Mobile; rv:43.0) Gecko/43.0 Firefox/43.0

Comment 12

[Max Ivanov[:MaxIvanov]]

I can't reproduce this on Flame 2.2

Device: Flame 2.2
Build ID: 20150901123002
Gaia: 335cd8e79c20f8d8e93a6efc9b97cc0ec17b5a46
Gecko: c03e2bc6a3a4
Gonk: bd9cb3af2a0354577a6903917bc826489050b40d
Version: 37.0 (2.2)
Firmware Version: v18D
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

Comment 13

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

Let's get a window on this issue since it wasn't occuring in 2.2.

Comment 14

[Martin Shuman [:Marty]]

I am able to reproduce this issue on Flame 2.5, 2.2, 2.1, and 2.0 builds.
When attempting to share an image from the Gallery to Twitter, the Twitter app will open, then the user will be returned to the Gallery app.

Note: On more recent 2.5 builds, this issue will also close out the Twitter app, but on earlier builds, the Twitter app will remain open in Card View, and be running in the background. Regardless, the improper behavior appears the same to the user.

Removing the regression and regressionwindow-wanted tags.

Environmental Variables:
Device: Flame 2.5
BuildID: 20150908030224
Gaia: 891798f1e345bc2b69e71de42bd524a90b1745c4
Gecko: 5fe9ed3edd6811a662d40d05e37b0d66e9520d82
Gonk: c4779d6da0f85894b1f78f0351b43f2949e8decd
Version: 43.0a1 (Master)
Firmware Version: v18D
User Agent: Mozilla/5.0 (Mobile; rv:43.0) Gecko/43.0 Firefox/43.0

Environmental Variables:
Device: Flame 2.2
BuildID: 20150908032505
Gaia: 335cd8e79c20f8d8e93a6efc9b97cc0ec17b5a46
Gecko: 96170c571381
Gonk: bd9cb3af2a0354577a6903917bc826489050b40d
Version: 37.0 (2.2)
Firmware Version: v18D
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

Environmental Variables:
Device: Flame 2.1
BuildID: 20150724001207
Gaia: 9dba58d18006e921546cec62c76074ce81e16518
Gecko: 41e10c6740be
Gonk: bd9cb3af2a0354577a6903917bc826489050b40d
Version: 34.0 (2.1)
Firmware Version: v18D
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Environmental Variables:
Device: Flame 2.0
BuildID: 20150723000207
Gaia: b16ba05481e577bc644ed8966f587a70fe2148e6
Gecko: 2e6f1d4deff9
Gonk: 040bb1e9ac8a5b6dd756fdd696aa37a8868b5c67
Version: 32.0 (2.0)
Firmware Version: v18D
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Comment 16

[Julien Wajsberg [:julienw]]

FWIW it works for me on the current dogfood build so I doubt it's bug 1049465.

However I get bug 1192703: the Twitter app goes to background and the Gallery app is foreground. However if I switch to the Twitter app, the image is ready to be shared.

Reopening.

Comment 17

[Julien Wajsberg [:julienw]]

[Blocking Requested - why for this release]:

Comment 18

[David Flanagan [:djf]]

Taking this to investigate

Comment 19

[David Flanagan [:djf]]

I see exactly what Dietrich reports in comment 0, using a Flame with a nightly build from a couple of days ago.  However, I also notice that the shared image does get transferred to the twitter app... I share the image, see twitter come up, and then I get taken right back to gallery.  But if I long press on the home button and go back into twitter, I am in the compose new tweet screen, and the image I shared is there, too.

Here's what I think is happening: the twitter app receives the share activity, gets the image blob, and then navigates from https://mobile.twitter.com to https://mobile.twitter.com/compose/tweet. I think the system app is seeing that navigation and assuming it means that the activity has failed and so is taking us back to the gallery app.

I see the same behavior when I share a photo from Camera to Twitter, so this is not a Gallery-specific bug. And since Twitter does receive and display the shared image, I think it is not a Twitter bug.  So I think this must be a System app bug.

Tim: in the past, I would have asked Alive to investigate this. Is there someone on your team who can take it?

Comment 20

[David Flanagan [:djf]]

I wonder if this regression was caused by bug 1192489 or bug 1190775?

Comment 21

[David Flanagan [:djf]]

Nope. I tried backing both of those out and the bug still occurs. I think we need professional help finding a regression window here.

In comment 8, Dietrich says it is a regression. In comment 14, Marty says it is not a regression and always occurs.  Maybe we're looking at two different bugs.

I think this is a regression and we need to find the window.

Comment 22

[David Flanagan [:djf]]

At one point this bug was marked as a duplicate of bug 1049465. They are not dupes. In that one, it looks like twitter opens up but does not display the image. In this one, twitter opens and displays the image, but then the user gets sent back to the gallery app. So when looking for a regression window, please keep the two separate.

Comment 23

[Tim Guan-tin Chien [:timdream] (please needinfo)]

There won't be a regression window if your comment 19 is correct. Twitter app is responsible of keeping on the current page (and the MozActivityRequestHandler instance) so that it could do handler.postResult() and complete the flow. When the instance is assumed lost Gecko assumes the activity handling has failed.

https://developer.mozilla.org/en-US/docs/Web/API/MozActivityRequestHandler

Unless my recollection on how "share" activity should work is wrong, and we don't ask inline activity apps for a response. If that's the case I don't really know when we *should* close the activity and *should not*.

It's sad that there is no documentation on these proprietary APIs, and sadder to keep using it 3 years after FxOS 1.0.

Comment 24

[Julien Wajsberg [:julienw]]

(In reply to David Flanagan [:djf] from comment #19)
> Here's what I think is happening: the twitter app receives the share
> activity, gets the image blob, and then navigates from
> https://mobile.twitter.com to https://mobile.twitter.com/compose/tweet.

Do you know if it is a real navigation or maybe a pushState call ?

Comment 25

[Etienne Segonzac (:etienne)]

(In reply to Julien Wajsberg [:julienw] (away Oct 1st, 2nd) from comment #24)
> (In reply to David Flanagan [:djf] from comment #19)
> > Here's what I think is happening: the twitter app receives the share
> > activity, gets the image blob, and then navigates from
> > https://mobile.twitter.com to https://mobile.twitter.com/compose/tweet.
> 
> Do you know if it is a real navigation or maybe a pushState call ?

I don't think the issue comes from the navigation anyway.

The Twitter app declares a share (disposition:window) activity with no returnValue but still does a |postResult('ok')|.

The postResult call triggers a mozbrowseractivitydone event, which triggers the transition back to the gallery app [1].

I doubt we'll get Twitter to do the fix but maybe gecko could ignore |postResult| calls when the activity has no returnValue?


[1] https://github.com/mozilla-b2g/gaia/blob/7507a12360ed14203b49c70c69e8b3950236c4f6/apps/system/js/app_window.js#L1010-L1013

Comment 26

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

There's no regression window to be found here.  We were able to reproduce this issue on all branches that we still make builds for in comment 14.

Comment 28

[Etienne Segonzac (:etienne)]

NI-ing Fabrice re Comment 25.

Comment 29

[[:fabrice] Fabrice Desré]

(In reply to Etienne Segonzac (:etienne) from comment #28)
> NI-ing Fabrice re Comment 25.

Sure, we can do that. Either rename this bug or file a new one.

Comment 30

[Ken Chang[:kenkai]]

Hi Etienne, Will you take this bug?

Comment 31

[Etienne Segonzac (:etienne)]

(In reply to Ken Chang[:ken] from comment #30)
> Hi Etienne, Will you take this bug?

Since I don't know anything about the gecko-side of the WebActivities implementation I hope not :)

Comment 32

[Andrew Sutherland [:asuth] (he/him)]

I can take this (I think ;) as part of our v2.5 blocker-fest, but if someone else would rather take it on, feel free to steal (promptly)!

Comment 33

[André Natal]

(In reply to Andrew Sutherland [:asuth] from comment #32)
> I can take this (I think ;) as part of our v2.5 blocker-fest, but if someone
> else would rather take it on, feel free to steal (promptly)!

Ok Andrew, I can take care of it!

Comment 34

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Andre Natal from comment #33)
> Ok Andrew, I can take care of it!

Cool.  So I think the fundamental issue is that ActivityWrapper does this no matter what:
  Services.obs.notifyObservers(docshell, "activity-done", aTopic);
Which BrowserElementChildPreload.js consumes and propagates it as "activitydone" to BrowserElementParent.js which in turn propagates it as a mozbrowser event per comment 25.

The simplest fix is that ActivityWrapper can conditionalize that statement by doing something like "if (aMessage.target.returnValue)".  It seems a bit sketchy to me, but I think it satisfies comment 25 and comment 29.

Comment 35

[Mahendra Potharaju [:mahe]]

Andre, Is this in progress with a fix?

Thanks

Comment 36

[André Natal]

(In reply to Mahendra Potharaju [:mahe] from comment #35)
> Andre, Is this in progress with a fix?
> 
> Thanks

Yes, it is. Will submit it today.

Comment 37

[GitHub Autolander Bot]

Attachment: [gaia] andrenatal:activities_noresult > mozilla-b2g:master

Comment 38

[Andre Natal]

Attachment: This patch contains the gecko part of the fix.

Try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=057b3608b22b

Comment 39

[[:fabrice] Fabrice Desré]

Comment on attachment 8679958 [details] [diff] [review]
This patch contains the gecko part of the fix.

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

I'd rather block postResult() from doing anything instead. Look if you can enforce that in https://mxr.mozilla.org/mozilla-central/source/dom/activities/ActivityRequestHandler.js#51

Also, we will need tests. There are some to get started in https://mxr.mozilla.org/mozilla-central/source/dom/activities/tests/mochi/

Comment 40

[André Natal]

Attachment: 8679958: This patch contains the gecko part of the fix.

Comment 41

[André Natal]

(In reply to [:fabrice] Fabrice Desré from comment #39)

> 
> I'd rather block postResult() from doing anything instead. Look if you can
> enforce that in
> https://mxr.mozilla.org/mozilla-central/source/dom/activities/
> ActivityRequestHandler.js#51
>

Fabrice, 

I couldn't find a way to access the returnValue in ActivityRequestHandler.js. 'aResult' from postResult can't be used, since is always filled in this particular situation we want to avoid with this bug [1].

In the tests I did, I realized the 'data' field of the payload that is carried in the message from ActivityWrapper is always 'null' no matter what. So I found less invasive simple include the returnValue there [2] and check it on ActivityRequestHandler [3] to then decide if we shall send or not the 'Activity:PostResult' message back.

That way we don't need to change the webidl to accommodate it. Another possibility is try to get the whole message by the id here [4], but I don't know if this is possible.

Here is the try for the new patch [5]

[1]
https://github.com/andrenatal/gecko-dev-speech/commit/b2e4723013989f07920663d2358fa414a41135cb#diff-3d4f1e7212797e921b15f76110cddf93L51

[2]
https://github.com/andrenatal/gecko-dev-speech/commit/b2e4723013989f07920663d2358fa414a41135cb#diff-00d7a0a493ef78c5fc41a6dbd70c63f0R43 

[3] https://github.com/andrenatal/gecko-dev-speech/commit/b2e4723013989f07920663d2358fa414a41135cb#diff-3d4f1e7212797e921b15f76110cddf93R52

[4] https://github.com/andrenatal/gecko-dev-speech/blob/b2e4723013989f07920663d2358fa414a41135cb/dom/activities/ActivityRequestHandler.js#L40
    
[5] https://treeherder.mozilla.org/#/jobs?repo=try&revision=8e8ca2fce262
 
> Also, we will need tests. There are some to get started in
> https://mxr.mozilla.org/mozilla-central/source/dom/activities/tests/mochi/

I wrote a new marionettejs test for it here [6] (or Comment 37) that is both passing with the patch and failing without it as you can see in gaia-try [7]. 

I tried to run these tests you mentioned in 'dom/activities/tests/mochi' with my b2g-desktop build, but I am getting a message saying there are no tests in this folder [8]. I am used to run mochitests for Desktop, but not for b2g, so I am not sure if I need something special. Mochitests from other folders (like dom/svg for example) runs fine.

So the marionettejs ones I already wrote are not enough?


[6] https://github.com/mozilla-b2g/gaia/pull/32805

[7] https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=19b4b93498327487c5f5894929122cd6ad798d56

[8]
mozillas-MacBook-Pro:gecko-b2g-desktop anatal$ ./mach mochitest dom/activities/tests/mochi/
From _tests: Kept 36895 existing; Added/updated 0; Removed 0 files and 0 directories.
The mochitest command could not find any supported tests to run! The
following flavors and subsuites were found, but are either not supported on
b2g_desktop builds, or were excluded on the command line:

    mochitest -f chrome (requires firefox or mulet or b2g or android)

Double check the command line you used, and make sure you are running in
context of the proper build. To switch build contexts, either run |mach|
from the appropriate objdir, or export the correct mozconfig:

    $ export MOZCONFIG=path/to/mozconfig



Thank you very much.

Comment 42

[[:fabrice] Fabrice Desré]

Comment on attachment 8680564 [details] [diff] [review]
8679958: This patch contains the gecko part of the fix.

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

::: dom/activities/ActivityWrapper.js
@@ +36,5 @@
>      // ActivitiesService will be able to fire an error and complete the
>      // Activity workflow.
>      cpmm.sendAsyncMessage("Activity:Ready", { id: aMessage.id });
>  
> +    // Bug 1194525 - Gecko should ignore |postResult| calls for WebActivities with no returnValue

nit: no need to add the bug number here.

@@ +39,5 @@
>  
> +    // Bug 1194525 - Gecko should ignore |postResult| calls for WebActivities with no returnValue
> +    // We add the return value to the payload to be send to ActivityRequestHandler properly
> +    // decide if should call postResult or not
> +    aMessage.payload.data = { returnValue: aMessage.target.returnValue };

You can't do that, because payload.data may be something else.

@@ +45,1 @@
>      let handler = new aWindow.ActivityRequestHandler(aMessage.id, aMessage.payload);

The clean thing to do is to pass returnValue here as an additional parameter to ActivityRequestHandler

Comment 43

[[:fabrice] Fabrice Desré]

I'm running tests usually with a mulet or firefox desktop build. But I'm fine with a marionnette test.

Comment 44

[André Natal]

Attachment: This patch contains the gecko part of the fix.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab3088cb045c

Comment 45

[André Natal]

(In reply to [:fabrice] Fabrice Desré from comment #42)
> Comment on attachment 8680564 [details] [diff] [review]
> 8679958: This patch contains the gecko part of the fix.
> 
> Review of attachment 8680564 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/activities/ActivityWrapper.js
> @@ +36,5 @@
> >      // ActivitiesService will be able to fire an error and complete the
> >      // Activity workflow.
> >      cpmm.sendAsyncMessage("Activity:Ready", { id: aMessage.id });
> >  
> > +    // Bug 1194525 - Gecko should ignore |postResult| calls for WebActivities with no returnValue
> 
> nit: no need to add the bug number here.
> 
> @@ +39,5 @@
> >  
> > +    // Bug 1194525 - Gecko should ignore |postResult| calls for WebActivities with no returnValue
> > +    // We add the return value to the payload to be send to ActivityRequestHandler properly
> > +    // decide if should call postResult or not
> > +    aMessage.payload.data = { returnValue: aMessage.target.returnValue };
> 
> You can't do that, because payload.data may be something else.
> 
> @@ +45,1 @@
> >      let handler = new aWindow.ActivityRequestHandler(aMessage.id, aMessage.payload);
> 
> The clean thing to do is to pass returnValue here as an additional parameter
> to ActivityRequestHandler

Thanks Fabrice. I was trying to avoid touching the constructor but actually is better to in this particular case.

Comment 46

[[:fabrice] Fabrice Desré]

Comment on attachment 8680861 [details] [diff] [review]
This patch contains the gecko part of the fix.

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

r=me with nit addressed

::: dom/activities/ActivityRequestHandler.js
@@ +40,3 @@
>      this._id = aId;
>      this._options = aOptions;
> +    this.returnValue = aReturnValue;

nit: this._returnValue to match the other private members.

Comment 47

[[:fabrice] Fabrice Desré]

Comment on attachment 8680861 [details] [diff] [review]
This patch contains the gecko part of the fix.

Jonas, can you r+ the webidl change. We needed to add a parameter to a chrome only constructor.

Comment 48

[André Natal]

Attachment: This patch contains the gecko part of the fix.

Comment 49

[André Natal]

(In reply to [:fabrice] Fabrice Desré from comment #47)
> Comment on attachment 8680861 [details] [diff] [review]
> This patch contains the gecko part of the fix.
> 
> Jonas, can you r+ the webidl change. We needed to add a parameter to a
> chrome only constructor.

Thanks Fabrice, just fixed the nit.

Comment 50

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8680876 [details] [diff] [review]
This patch contains the gecko part of the fix.

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

Webidl changes look fine.

Comment 51

[[:fabrice] Fabrice Desré]

Andre,

You need to provide a hg patch and fix the commit message (r=fabrice,sicking)

Comment 52

[André Natal]

Attachment: This patch contains the gecko part of the fix.

Comment 53

[André Natal]

(In reply to [:fabrice] Fabrice Desré from comment #51)
> Andre,
> 
> You need to provide a hg patch and fix the commit message (r=fabrice,sicking)

Yes, thanks and sorry about that. Just fixed.

Comment 54

[André Natal]

Comment on attachment 8680991 [details]
This patch contains the gecko part of the fix.

Flagging it as the r+ since fabrice and sicking already flagged the obsolete ones.

Comment 55

[Carsten Book [:Tomcat]]

does the Gaia PR also need landing ?

Comment 56

[André Natal]

Yes, but just after the gecko patch land in m-c for the test in the pr then be able to pass in gaia-try

Comment 57

[André Natal]

So right now, only gecko patch.

Comment 58

[[:fabrice] Fabrice Desré]

In these cases you should rather split up the gaia patch in its own bug.

Comment 59

[Carsten Book [:Tomcat]]

(In reply to Andre Natal from comment #56)
> Yes, but just after the gecko patch land in m-c for the test in the pr then
> be able to pass in gaia-try

landed the gecko patch

Comment 60

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/54c770bf243e

Comment 61

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I backed this out in https://hg.mozilla.org/integration/mozilla-inbound/rev/b892fb047ebc for apparently causing a spike in gij(11/12) failures like https://treeherder.mozilla.org/logviewer.html#?job_id=16697770&repo=mozilla-inbound

Comment 62

[Pulsebot]

Backout:
https://hg.mozilla.org/mozilla-central/rev/9f69202d8275

Comment 63

[André Natal]

(In reply to Wes Kocher (:KWierso) from comment #61)
> I backed this out in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/b892fb047ebc for
> apparently causing a spike in gij(11/12) failures like
> https://treeherder.mozilla.org/logviewer.html#?job_id=16697770&repo=mozilla-
> inbound

Thank you Wes. 

I am figuring out what was the collateral effect in the ringtone app.

Comment 64

[Jim Porter (:squib)]

I must confess, I'm somewhat baffled at the decision here. Wasn't there a point where *both* the Twitter app and the Ringtones app worked fine? (This part is unclear, since no one could find a regression window, but I trust Dietrich that it worked at some point.) If this is Twitter's fault, do we know why they changed it? Comment 25 suggests that this is a bug on Twitter's end, but it doesn't seem like anyone's even given them a quick heads-up, on the assumption that they wouldn't care. It's possible that someone has tried pinging them, but I don't see any indication here.

If it's our fault (which seems unlikely from the discussion), shouldn't we try to restore the old logic instead of having activity semantics that require one of the apps to change? I assume the goal is for this API to be standardized at some point, so shouldn't we be clear on the exact semantics here? If this special-casing of the returnValue attribute is something we'd want standardized, I don't have a huge issue with making the change, but otherwise, we should be ensuring that our implementation behaves *correctly*. As bug 1221358 shows, this can break existing apps, and it doesn't seem like we've checked to see if this change would break anyone else.

Comment 65

[[:fabrice] Fabrice Desré]

In any case, we need to protect ourselves against pages that still call postResult() even if they claimed they would not. Blaming the app developer doesn't help the poor user too much... So I still feel this is a worthwhile fix on the gecko side.
I don't understand why you feel it's correct to allow postResult() in this situation.

Comment 66

[Andrew Sutherland [:asuth] (he/him)]

Providing some context for the ringtones scenario, it was relying on postResult() to trigger the window.close() of its "disposition: inline" activity.  Any app that depends on postResult to trigger window.close() is potentially going to have a bad day.

We could further gate things so that inline activities that call postResult will close, but not windowed activities.  This could make sense because an inline activity by definition cannot/should not stand on its own, but a windowed activity could indeed make sense.

Comment 67

[[:fabrice] Fabrice Desré]

So it seems we have two issues:
1) people calling postResult() when they said they wont.
2) "disposition: inline" activities needing a way to tell us they need to be closed which would not be a side effect of 1) when they are done. But isn't window.close() enough there?

Comment 68

[Jim Porter (:squib)]

I've always interpreted postResult as orthogonal to the presence of returnValue in the manifest. To me, postResult means "I'm done doing whatever I was doing in the activity", but it doesn't say anything about whether the caller should expect anything to be returned. As Andrew notes, in the Ringtones app, the goal here is for postResult to ultimately close the window, but using postResult is more explicit that the activity is "done". If other browsers implemented the Activity API, the distinction between window.close and activity.postResult may be more important. Personally, I don't have a strong opinion on which one we want to bless, provided we've made a conscious decision.

However, I think the bigger issue is that this is a change in behavior which could negatively affect other apps that do similar things to the Ringtones app. At the *very* least, if we take the attached patch, postResult should log an error (or even better, throw an exception) if returnValue isn't set. The current patch is a silent change in behavior that no one would notice without thorough integration tests.

Still, since I don't see any discussion about this issue with the Twitter folks, I have to assume they don't know it's a problem, and I think it would be wise to at least ask them what they were trying to do before we check in a patch to work around this issue. It could simply be that they didn't understand the semantics of this apparently-undocumented (see comment 23) API.

Comment 69

[[:fabrice] Fabrice Desré]

(In reply to Jim Porter (:squib) from comment #68)
> I've always interpreted postResult as orthogonal to the presence of
> returnValue in the manifest. To me, postResult means "I'm done doing
> whatever I was doing in the activity", but it doesn't say anything about
> whether the caller should expect anything to be returned. As Andrew notes,
> in the Ringtones app, the goal here is for postResult to ultimately close
> the window, but using postResult is more explicit that the activity is
> "done". If other browsers implemented the Activity API, the distinction
> between window.close and activity.postResult may be more important.
> Personally, I don't have a strong opinion on which one we want to bless,
> provided we've made a conscious decision.

I'm not a native speaker so bear with me (and naming is hard), but "postResult" sounds like a pretty straightforward way to say "I'm sending you back something". So I guess we'll have to agree that we disagree in our interpretation there, and that the real issue is that we need to make the API expectations clearer.
Also, there is no effort to try to standardize this api in its current form.

> However, I think the bigger issue is that this is a change in behavior which
> could negatively affect other apps that do similar things to the Ringtones
> app. At the *very* least, if we take the attached patch, postResult should
> log an error (or even better, throw an exception) if returnValue isn't set.
> The current patch is a silent change in behavior that no one would notice
> without thorough integration tests.

I agree with adding a noisy Cu.reportError(...) and throwing. André, can you update your patch?

Comment 70

[André Natal]

(In reply to [:fabrice] Fabrice Desré from comment #69)


> I agree with adding a noisy Cu.reportError(...) and throwing. André, can you
> update your patch?

Yes, sure. I agree that throwing an exception here can be useful for the developer. I will update it.

Comment 71

[Jim Porter (:squib)]

(In reply to [:fabrice] Fabrice Desré from comment #69)
> I'm not a native speaker so bear with me (and naming is hard), but
> "postResult" sounds like a pretty straightforward way to say "I'm sending
> you back something". So I guess we'll have to agree that we disagree in our
> interpretation there, and that the real issue is that we need to make the
> API expectations clearer.

That's fine. I based my interpretation on guesswork, since it's not documented. If postResult is defined to only make sense with returnValue set, that's ok by me (as long as that doesn't cause any other issues).

> Also, there is no effort to try to standardize this api in its current form.

That's disappointing. I thought our goal was to standardize all the Mozilla-specific APIs so that we only used standards-based technologies. That's a whole other set of issues, though.

> I agree with adding a noisy Cu.reportError(...) and throwing. André, can you
> update your patch?

We should probably make sure that throwing a real exception doesn't break Twitter (although part of me smiles at the idea of breaking bad code).

Comment 72

[André Natal]

Attachment: This patch contains the gecko part of the fix.

I confirm that with the Cu.reportError() call, both the Ringtone as the Twitter shows the error in the console [1], but only the latter doesn't break, different of the former that requires the landing of Bug 1221358 to avoid breaking.

Here is the try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6013afafc7c9 


[1] 

E/Twitter ( 2537): [JavaScript Error: "postResult() can't be called when 'returnValue': 'true' isn't declared in manifest.webapp" {file: "jar:file:///system/b2g/omni.ja!/components/ActivityRequestHandler.js" line: 60}]

E/Ringtones( 3044): [JavaScript Error: "postResult() can't be called when 'returnValue': 'true' isn't declared in manifest.webapp" {file: "jar:file:///system/b2g/omni.ja!/components/ActivityRequestHandler.js" line: 60}]

Comment 73

[Julien Wajsberg [:julienw]]

FWIW we use postResult in the SMS app as well, with the same expectation that the window will be closed. But I admit we have the expectation because it worked that way, not because it made sense.

Moreover, I see we actually have "returnValue" in our manifest, and I think we added it then because postResult was doing nothing in inline activities without this. So maybe something actually changed how postResult was working without "returnValue" at one point ?

Comment 74

[[:fabrice] Fabrice Desré]

André, did you try also throwing after Cu.reportError() ? If that breaks 3rd party sites too badly we may not do that, but we can fix gaia consumers.

Comment 75

[André Natal]

(In reply to [:fabrice] Fabrice Desré from comment #74)
> André, did you try also throwing after Cu.reportError() ? If that breaks 3rd
> party sites too badly we may not do that, but we can fix gaia consumers.

Fabrice, I just tested it with both Cu.reportError() and throw [1], and Twitter handled it but didn't broke. The image was shared and posted correctly [2], despite the errors in console [3].

If you say that's ok, I'll fix the patch and push to try again. 

[1]
    } else {
      Cu.reportError("postResult() can't be called when 'returnValue': 'true' isn't declared in manifest.webapp");
      throw new Error("postResult() can't be called when 'returnValue': 'true' isn't declared in manifest.webapp");
    }


[2] https://twitter.com/andrenatalbr/status/663667547984674816


[3] E/Twitter ( 1826): [JavaScript Error: "postResult() can't be called when 'returnValue': 'true' isn't declared in manifest.webapp" {file: "jar:file:///system/b2g/omni.ja!/components/ActivityRequestHandler.js" line: 60}]
E/Twitter ( 1826): [JavaScript Error: "Error: postResult() can't be called when 'returnValue': 'true' isn't declared in manifest.webapp" {file: "jar:file:///system/b2g/omni.ja!/components/ActivityRequestHandler.js" line: 61}]
E/Twitter ( 1826): [JavaScript Error: "NS_ERROR_UNEXPECTED: " {file: "https://ma.twimg.com/twitter-mobile/15f39f5f185f49a822b85557b539cf2779511d87/assets/m5.js" line: 6}]

Comment 76

[André Natal]

(In reply to [:fabrice] Fabrice Desré from comment #74)
> André, did you try also throwing after Cu.reportError() ? If that breaks 3rd
> party sites too badly we may not do that, but we can fix gaia consumers.

And this was the error in Ringtone app:

E/Ringtones( 2891): [JavaScript Error: "postResult() can't be called when 'returnValue': 'true' isn't declared in manifest.webapp" {file: "jar:file:///system/b2g/omni.ja!/components/ActivityRequestHandler.js" line: 60}]
E/Ringtones( 2891): [JavaScript Error: "Error: postResult() can't be called when 'returnValue': 'true' isn't declared in manifest.webapp" {file: "jar:file:///system/b2g/omni.ja!/components/ActivityRequestHandler.js" line: 61}]
E/Ringtones( 2891): [JavaScript Error: "NS_ERROR_UNEXPECTED: " {file: "app://ringtones.gaiamobile.org/js/manage.js" line: 12}]

Comment 77

[André Natal]

Attachment: This patch contains the gecko part of the fix.

Updated version of the patch containing the throw.

Comment 78

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2433822aeda2

Comment 79

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2433822aeda2

Comment 80

[Gregor Wagner [:gwagner]]

Hm why isn't this uplifted to the 2.5 branch?

Comment 81

[André Natal]

(In reply to Gregor Wagner [:gwagner] from comment #80)
> Hm why isn't this uplifted to the 2.5 branch?

I realized this too but I see Mahe already had flagged it to uplift so I preferred to wait

Comment 82

[Julien Wajsberg [:julienw]]

Well, this simply needs an approval request from André...

Comment 83

[Mahendra Potharaju [:mahe]]

Andre - I don't see the request flag.. Can you please set it and answer the questions that follow. 

Thanks

Comment 84

[André Natal]

Comment on attachment 8685147 [details] [diff] [review]
This patch contains the gecko part of the fix.

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 1194525 
User impact if declined: Apps (like Twitter) that doesn't correctly implement returnValue can fail 
Testing completed:  https://treeherder.mozilla.org/#/jobs?repo=try&revision=6013afafc7c9 
Risk to taking this patch (and alternatives if risky): -  
String or UUID changes made by this patch: -

Comment 85

[André Natal]

(In reply to Mahendra Potharaju [:mahe] from comment #83)
> Andre - I don't see the request flag.. Can you please set it and answer the
> questions that follow. 
> 
> Thanks

Hi Mahe. Just set it. 
Thanks

Comment 86

[Dylan Oliver [:doliver]]

Let's hold off on uplift for right now -- Andre is investigating if bug 1221379 is a regression introduced by this fix.

Comment 87

[Mahendra Potharaju [:mahe]]

Comment on attachment 8685147 [details] [diff] [review]
This patch contains the gecko part of the fix.

Sure. Thanks Dylan. Removed from request queue. 

Andre - Please set the flag once regression cause is identified and its safe for this patch to land. 

Thanks

Comment 88

[Julien Wajsberg [:julienw]]

Dylan, Mahendra, as far as I understand, bug 1221379 does not impact the user code, and not even the tests that test the user code...

Comment 89

[Dylan Oliver [:doliver]]

(In reply to Julien Wajsberg [:julienw] from comment #88)
> Dylan, Mahendra, as far as I understand, bug 1221379 does not impact the
> user code, and not even the tests that test the user code...

Sorry! I pasted the wrong bug in there. The regression is bug 1224130.

Comment 90

[Julien Wajsberg [:julienw]]

Ah yeah, this one looks likely :D

Comment 91

[André Natal]

Bug 1224130 was fixed 20 days ago by Bug 1224395. 

So I think we are good to land this one (Bug 1194525).

Comment 92

[Julien Wajsberg [:julienw]]

André, don't forget we'll need to uplift bug 1224395 too, then.

Comment 93

[André Natal]

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 1194525 
User impact if declined: Apps (like Twitter) that doesn't correctly implement returnValue can fail 
Testing completed:  https://treeherder.mozilla.org/#/jobs?repo=try&revision=6013afafc7c9 
Risk to taking this patch (and alternatives if risky): -  
String or UUID changes made by this patch: -

Comment 94

[André Natal]

(In reply to Julien Wajsberg [:julienw] from comment #92)
> André, don't forget we'll need to uplift bug 1224395 too, then.

Thanks Julien. I just flagged both to uplift.

Comment 96

[Mahendra Potharaju [:mahe]]

Andre - Can you please confirm there are no string changes in this patch?

Thanks

Comment 97

[André Natal]

Mahe, confirmed.

Comment 98

[Mahendra Potharaju [:mahe]]

Comment on attachment 8685147 [details] [diff] [review]
This patch contains the gecko part of the fix.

Approved for 2.5 uplift. Thanks

Comment 99

[Christof Meerwald]

Just found this bug based on the error message "Error: postResult() can't be called when 'returnValue': 'true' isn't declared in manifest.webapp" I got.

I am actually still not entirely sure what I should do in my application - call "window.close" instead? But that then seems to trigger "NS_ERROR_ILLEGAL_VALUE: Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIMessageSender.sendAsyncMessage]"

The other question is how is anyone ever going to find out what to do? The documentation at https://developer.mozilla.org/en-US/docs/Web/API/MozActivityRequestHandler definitely isn't sufficient. And to me it seems very strange to use window.close instead of a MozActivityRequestHandler method. My guess was to post a "null" result back, but that doesn't work any more now.

Comment 100

[Julien Wajsberg [:julienw]]

There are 2 solutions for you:
* either you add "returnValue: true" in your manifest, so that you can use postResult
* or you use window.close() because you don't have anything to return.

I think you can safely ignore the "NS_ERROR_ILLEGAl_VALUE" error, but still please file a separate bug for this one.

About developer.mozilla.org: well, it's a wiki, feel free to edit :) Actually we'd be glad ! (maybe more in https://developer.mozilla.org/en-US/docs/Web/API/Web_Activities#Handle_an_activity though ?)

Comment 101

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/6475f64054d5

Comment 102

[Chris Mills [:cmills]]

I've added the following note to the articles in question:

Be aware that you need to have returnValue: true set in your manifest file to return a result (see manifest activities for more information.) If there is no result to return, then you should just use {{domxref("window.close()")}} to get rid of the handling window.

See:

https://developer.mozilla.org/en-US/docs/Mozilla/Firefox_OS/API/Web_Activities#Handle_an_activity
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox_OS/API/MozActivityRequestHandler