584401 - Prompts are not accessible to child process C++

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Egor Starkov]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.19) Gecko/2010031218 Firefox/3.5
Build Identifier: 

When e10s fennec get some load error it tries to invoke alert from nsDocShell::DisplayLoadError using nsIPrompt from nsPrompter.js. But this doesn't work because not implemented for non-e10s fennec. Now fennec has kinda workaround impelented in bug #573635 that works only for page JS alerts.
My guess is that we need to remove PromptRemoter and implement conditional e10s code in nsPrompter.js.

Now i'm stuck in sending message to parent process. sendAsyncMessage is not defined in Prompter. How this can be done?

Reproducible: Always

Steps to Reproduce:
1. Open any invalid protocol, for example asd://xcv. 

Actual Results:  
Nothing happens.

Expected Results:  
Alert is displayed.

Comment 1

[Tony Chung [:tchung]]

without e10s enabled on fennec n900, i get a message saying "Firefox doesn't know how to open this address, because the protocol (asd) isn't associated with any program."  

Will retest e10s is back on

Comment 2

[Aakash Desai [:aakashd]]

I didn't receive a message using a new profile on builds:

Mozilla/5.0 (X11; U; Linux armv71; Nokia N900; en-US; rv:2.0b4pre) Gecko/20100810 Namoroka/4.0b4pre Fennec/2.0a1pre

and

Mozilla/5.0 (Android; U; Linux armv71; en-US; rv:2.0b4pre) Gecko/20100810 Namoroka/4.0b4pre Fennec/2.0a1pre

Comment 3

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

Egor - In JS XPCOM components, we can use the message manager like this:
https://wiki.mozilla.org/Mobile/Fennec/Extensions/Electrolysis#Working_in_JS_XPCOM

Comment 4

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

Note: This is true for any access to nsIPromptService from C++ in the child process.

Comment 5

[Alon Zakai (:azakai)]

Attachment: Part 1: Remote prompts in PromptService

Comment 6

[Alon Zakai (:azakai)]

Attachment: Part 2: Undo previous remoting code from bug 573635

Comment 7

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

Alon - If I am not mistaken, the code changes would be much cleaner, and potentially only in PromptService.js, if we wait for bug 585173 to land.

I'll take a review pass through the patches, just in case bug 585173 doesn't land soon enough for beta 1.

Comment 8

[Alon Zakai (:azakai)]

(In reply to comment #7)
> Alon - If I am not mistaken, the code changes would be much cleaner, and
> potentially only in PromptService.js, if we wait for bug 585173 to land.
> 
> I'll take a review pass through the patches, just in case bug 585173 doesn't
> land soon enough for beta 1.

The difference would be just a few lines - I wrote a comment in the patch which lines can be changed, should bug 585173 be ready in time.

But actually that bug really isn't needed here. It won't lead to shorter or safer code. Maybe slightly less hackish though.

Comment 9

[Alon Zakai (:azakai)]

Attachment: Part 1 v2

Patch updated to use bug 585173, which looks like it is close to landing.

Comment 10

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

Comment on attachment 469678 [details] [diff] [review]
Part 1 v2

Can't you move the PromptServiceRemoter into the PromptService,js component?

Comment 11

[Alon Zakai (:azakai)]

(In reply to comment #10)
> Comment on attachment 469678 [details] [diff] [review]
> Part 1 v2
> 
> Can't you move the PromptServiceRemoter into the PromptService,js component?

Well yes, but then the PromptService component needs to be loaded in the parent which is slower. And anyhow both approaches would be temporary solutions until bug 591052. So I suggest leaving this for then. I added a comment in that bug about which components we should utilize it for when it is done.

Comment 12

[Doug Turner (:dougt)]

no hacks, lets just fix 591052 asap.

Comment 13

[Alon Zakai (:azakai)]

Attachment: Part 1 v3

Ok, updated patch using bug 591052, and moved entirely to PromptService. No hacks, all clean.

Comment 14

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

Comment on attachment 470089 [details] [diff] [review]
Part 1 v3


>diff --git a/components/PromptService.js b/components/PromptService.js
> 
>+var gPromptService = null;
>+
> function PromptService() {
>+  // Depending on if we are in the parent or child, prepare to remote
>+  // certain calls
>+  var appInfo = Cc["@mozilla.org/xre/app-info;1"];
>+  if (!appInfo || appInfo.getService(Ci.nsIXULRuntime).processType ==
>+      Ci.nsIXULRuntime.PROCESS_TYPE_DEFAULT) {

Feel free to not wrap

>+        case "Prompt:Call":
>+          // List of methods we remote - to check against malicious data
>+          const ALL_METHODS = ['alert', 'confirm', 'prompt'];

why only these methods? we'll need to support all the prompt methods from c++ right?

>+          var method = aMessage.json.method;
>+          if (ALL_METHODS.indexOf(method) == -1) {
>+            throw 'PromptServiceRemoter received an invalid method';
>+          }

No {} for a single line and do we really need this check?

>+          arguments.push(ret);
>+          return arguments;
>+          break;

No need for the break after a return

>+    this.messageManager = Cc["@mozilla.org/childprocessmessagemanager;1"].
>+                          getService(Ci.nsISyncMessageSender);

Feel free to not wrap

>   alert: function alert(aTitle, aText) {
>+    if (gPromptService.inContentProcess) {
>+      return gPromptService.callProxy("alert", ['Alert'].concat(Array.prototype.slice.call(arguments, 0)));
>+    }

Why only modify "alert"? Do we need to do this at all? Won't the callProxy handle what we need?

looking pretty good

Comment 15

[Alon Zakai (:azakai)]

Attachment: Part 1 v4

Updated patch following comments and irc discussion (mainly better comments). Also mention temporary workaround with wrappedJSObject.

Comment 16

[Alon Zakai (:azakai)]

Attachment: Part 2 v2

Rebased patch for undoing old remoting code from bug 573635.

Comment 17

[Matt Brubeck (:mbrubeck)]

http://hg.mozilla.org/mobile-browser/rev/e305f7103d0a
http://hg.mozilla.org/mobile-browser/rev/cf490bf0fa1a

Comment 18

[Justin Wood (:Callek)]

from the looks of things, no longer c-n

Comment 19

[Cristian Nicolae (:xti)]

Verified fixed on:
Mozilla/5.0 (Android;Linux armv7l;rv:9.0a1)Gecko/20110919
Firefox/9.0a1 Fennec/9.0a1
Device: Samsung Galaxy S
OS: Android 2.2