If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Double prompts and bad response for Prompt:Call message

VERIFIED FIXED

Status

Fennec Graveyard
General
VERIFIED FIXED
7 years ago
6 years ago

People

(Reporter: mbrubeck, Assigned: azakai)

Tracking

Bug Flags:
in-testsuite ?

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

7 years ago
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.
(Reporter)

Comment 1

7 years ago
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.
(Assignee)

Comment 2

7 years ago
> 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.
Does this mean we don't need the .wrappedJSObject workaround either?!?! That would be cool.
(Assignee)

Comment 4

7 years ago
(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.
(Reporter)

Comment 5

7 years ago
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.
Assignee: nobody → mbrubeck
Status: NEW → ASSIGNED
Attachment #473704 - Flags: review?(mark.finkle)
(Reporter)

Comment 6

7 years ago
Created attachment 473708 [details] [diff] [review]
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]
************************************************************
Attachment #473708 - Flags: review?(azakai)
(Assignee)

Comment 7

7 years ago
(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.
(Reporter)

Comment 8

7 years ago
(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.
(Assignee)

Comment 9

7 years ago
(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 on attachment 473704 [details] [diff] [review]
mobile-browser patch

So it seems this patch is still what we need for now.
Attachment #473704 - Flags: review?(mark.finkle) → review+
(Assignee)

Comment 11

7 years ago
(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 on attachment 473704 [details] [diff] [review]
mobile-browser patch

Let's do the timeout approach. Alon has convinced me
Attachment #473704 - Flags: review+ → review-
(Assignee)

Comment 13

7 years ago
Created attachment 474257 [details] [diff] [review]
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.
(Reporter)

Comment 14

7 years ago
(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. :(
(Assignee)

Comment 15

7 years ago
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.
tracking-fennec: --- → ?
Flags: in-testsuite?
(Assignee)

Comment 16

7 years ago
Created attachment 474775 [details] [diff] [review]
Timeouts in Wakeup Service v2

Yay, a proper fix for the wakeup service problem. No more duplicate prompts or errors.
Attachment #474257 - Attachment is obsolete: true
Attachment #474775 - Flags: review?(mark.finkle)
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" :)
Attachment #474775 - Flags: review?(mark.finkle) → review+
(Assignee)

Comment 18

7 years ago
Created attachment 474870 [details] [diff] [review]
Timeouts in Wakeup Service v3

Patch for checkin.
Attachment #473704 - Attachment is obsolete: true
Attachment #473708 - Attachment is obsolete: true
Attachment #474775 - Attachment is obsolete: true
Attachment #474870 - Flags: review+
Attachment #473708 - Flags: review?(azakai)
(Assignee)

Updated

7 years ago
Assignee: mbrubeck → azakai

Updated

7 years ago
tracking-fennec: ? → 2.0b2+
Duplicate of this bug: 598882
Keywords: checkin-needed
Whiteboard: [fennec-checkin-postb1]
(Assignee)

Comment 20

7 years ago
http://hg.mozilla.org/mozilla-central/rev/c0a19a64d7ac
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [fennec-checkin-postb1]
Is it Bug 641876 related to this one?

Comment 22

6 years ago
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.