594845 - Double prompts and bad response for Prompt:Call message

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Matt Brubeck (:mbrubeck)]

Steps to reproduce:
1. Visit http://mycroft.mozdev.org/google-search-plugins.html
2. Click any of the search engine links (e.g. "Google").

Expected results: After a prompt, the new search engine is installed.

Actual results: The first time you do this after Fennec launches, two prompts appear instead of one.  (Only one prompt appears if you try it again.)  Also, the search engine installation fails every time with these errors:

[Exception... "'[JavaScript Error: "response is undefined" {file: "file:///home/mbrubeck/src/mozilla/fennec/dist/bin/components/PromptService.js" line: 161}]' when calling method: [nsIPromptService::confirmEx]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: file:///home/mbrubeck/src/mozilla/fennec/dist/bin/components/nsSearchService.js :: SRCH_SVC_confirmAddEngine :: line 1193"  data: yes]
************************************************************
[Exception... "Component returned failure code: 0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE) [nsIJSCID.getService]"  nsresult: "0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE)"  location: "JS frame :: file:///home/mbrubeck/src/mozilla/fennec/dist/bin/components/nsSearchService.js :: SRCH_SVC__buildCache :: line 2448"  data: no]

It looks like sendSyncMessage("Prompt:Call", ...) is returning an array with two responses, and the first is undefined.

Comment 1

[Matt Brubeck (:mbrubeck)]

The problem seems to be this:

MessageWakeupService.prototype.receiveMessage calls addMessageListener(name, service).  Then it calls service.receiveMessage directly.  Then messageManager *also* calls service.receiveMessage, because the the listener has already been added.

Commenting out the service.receiveMessage call in MWS fixes the double prompt problem, but we might not want to rely on this behavior.  Maybe it would be better to call addMessageListener in a timeout, or to continue to pass all messages through MWS.

Also, MWS.prototype.receiveMessage should return the result of service.receiveMessage.

Comment 2

[Alon Zakai (:azakai)]

> Then messageManager
*also* calls service.receiveMessage, because the the listener has already been
added.

Yeah, I believe you are right. Probably this happens because all of this was inside a loop in the messagemanager code, on the listeners, and we just added something to that list - and the messagemanager doesn't clone it before iterating.

Relying on that behavior sounds bad, I agree. But a timeout would re-order messages, which might mess some things up. And continuing to pass all messages through the MWS would add overhead to each call. Also fixing the messagemanager to clone the list each time would add overhead.

So I would be in favor or relying on the behavior. Automatic tests should catch if the behavior changes, in which case it would be a 1-line fix in just the MWS to handle that.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Does this mean we don't need the .wrappedJSObject workaround either?!?! That would be cool.

Comment 4

[Alon Zakai (:azakai)]

(In reply to comment #3)
> Does this mean we don't need the .wrappedJSObject workaround either?!?! That
> would be cool.

Ironically yes... but we are just juggling hacks here.

Comment 5

[Matt Brubeck (:mbrubeck)]

Attachment: mobile-browser patch

Messages that pass through the MessageWakeupService have an extra listener that returns "undefined" in the response array.  We need to filter out this extra response when calling sendSyncMessage.

Comment 6

[Matt Brubeck (:mbrubeck)]

Attachment: mozilla-central patch: remove explicit service.receiveMessage

I tried to remove the wrappedJSObject hack too, but it caused errors.

Note: Even with these patches, adding a search engine still does not work.  There is an unrelated error; we probably need to do some additional work to remote the search service:

************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIProperties.get]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: file:///home/mbrubeck/src/mozilla/fennec/dist/bin/components/nsSearchService.js :: getDir :: line 424"  data: no]
************************************************************
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "Component returned failure code: 0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE) [nsIJSCID.getService]"  nsresult: "0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE)"  location: "JS frame :: file:///home/mbrubeck/src/mozilla/fennec/dist/bin/components/nsSearchService.js :: SRCH_SVC__buildCache :: line 2448"  data: no]
************************************************************

Comment 7

[Alon Zakai (:azakai)]

(In reply to comment #5)
> Created attachment 473704 [details] [diff] [review]
> mobile-browser patch
> 
> Messages that pass through the MessageWakeupService have an extra listener that
> returns "undefined" in the response array.  We need to filter out this extra
> response when calling sendSyncMessage.

If we fix the duplicate messages (for example by relying on the current MM behavior & fixing the wakeup service accordingly), we won't need this hack - so I don't understand why this patch is needed.

Comment 8

[Matt Brubeck (:mbrubeck)]

(In reply to comment #7)
> If we fix the duplicate messages (for example by relying on the current MM
> behavior & fixing the wakeup service accordingly), we won't need this hack - so
> I don't understand why this patch is needed.

If we remove the "service.receiveMessage" call and rely on current MM behavior, then there are still two listeners during the first message, so sendSyncMessage still gets two responses.

Comment 9

[Alon Zakai (:azakai)]

(In reply to comment #8)
> (In reply to comment #7)
> > If we fix the duplicate messages (for example by relying on the current MM
> > behavior & fixing the wakeup service accordingly), we won't need this hack - so
> > I don't understand why this patch is needed.
> 
> If we remove the "service.receiveMessage" call and rely on current MM behavior,
> then there are still two listeners during the first message, so sendSyncMessage
> still gets two responses.

Ah. Then that sounds bad, it's a good reason to not rely on the current MM behavior.

Maybe we should just use a timeout as suggested earlier. Services using the wakeup service would need to know about minor reordering issues, but that might not be too bad.

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 473704 [details] [diff] [review]
mobile-browser patch

So it seems this patch is still what we need for now.

Comment 11

[Alon Zakai (:azakai)]

(In reply to comment #10)
> Comment on attachment 473704 [details] [diff] [review]
> mobile-browser patch
> 
> So it seems this patch is still what we need for now.

I disagree. I think we should go with the timeout solution in the wakeup service. It would prevent the need for the hack in this patch - which would be needed in everything that wants to use the wakeup service.

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 473704 [details] [diff] [review]
mobile-browser patch

Let's do the timeout approach. Alon has convinced me

Comment 13

[Alon Zakai (:azakai)]

Attachment: Timeouts in Wakeup Service

Patch to fix the wakeup service, using a timeout, so we would not get the duplication any more.

Haven't had the opportunity to test it yet, so not asking for review, but I think it should work.

Comment 14

[Matt Brubeck (:mbrubeck)]

(In reply to comment #13)
> Created attachment 474257 [details] [diff] [review]
> Timeouts in Wakeup Service

I still get two prompts when using this patch, for some reason. :(

Comment 15

[Alon Zakai (:azakai)]

Sadly, there are some problems here. Showing a modal dialog in the parent runs the event loop, from inside the messagemanager call, so even using a timeout doesn't help us escape from that (unless it is so long that the user already closed the dialog...). In other words we do get to the main event loop, but it is being called from inside the messagemanager call. We're still inside the matrix.

Even an async call from the child will not fix this.

Ideas at this point: Do some convoluted back&forth messaging in order to escape properly. Or, just give in and continue remoting calls through the wakeup service, and suffer the performance penalty.

Comment 16

[Alon Zakai (:azakai)]

Attachment: Timeouts in Wakeup Service v2

Yay, a proper fix for the wakeup service problem. No more duplicate prompts or errors.

Comment 17

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 474775 [details] [diff] [review]
Timeouts in Wakeup Service v2

>diff --git a/content/base/src/messageWakeupService.js b/content/base/src/messageWakeupService.js

>   receiveMessage: function(aMessage) {

>+    if (data.timer) {
>+      // Handle the case of two such messages happening in quick succession
>+      data.timer.cancel();
>+      data.timer = null;
>+    }
>+    data.timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);

Add a blank line after the "if" block

Use more "let" and less "var" :)

Comment 18

[Alon Zakai (:azakai)]

Attachment: Timeouts in Wakeup Service v3

Patch for checkin.

Comment 20

[Alon Zakai (:azakai)]

http://hg.mozilla.org/mozilla-central/rev/c0a19a64d7ac

Comment 21

[Cristian Nicolae (:xti)]

Is it Bug 641876 related to this one?

Comment 22

[Carla Nadastean]

Retested bug with:
Build id : Mozilla/5.0 (Android;Linux armv7l; rv:9.0a1)Gecko/20110926
Firefox/9.0a1 Fennec /9.0a1
Device: Motorola DROID 2
OS: Android 2.3

Bug is no longer reproducible:

After one search engine is tapped, only one prompt message is displayed.

After selecting add, on the prompt message, search engine is successfully added.

NOTE: If another search engine is tapped before the prompt message is displayed, 2 prompts are displayed. Issue is tracked in Bug 641876 - Popup messages overlap if two different search engine links are tapped.

Verifying bug.