Prompts are not accessible to child process C++

VERIFIED FIXED

Status

Firefox for Android Graveyard
General
VERIFIED FIXED
8 years ago
7 years ago

People

(Reporter: Egor Starkov, Assigned: azakai)

Tracking

Trunk
Other
Linux
Dependency tree / graph
Bug Flags:
in-testsuite ?

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

8 years ago
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

8 years ago
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
Status: UNCONFIRMED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → WORKSFORME
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
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: WORKSFORME → ---
tracking-fennec: --- → ?
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
Summary: Unknown protocol address is not properly handled by browser → Prompts are not accessible to child process C++
tracking-fennec: ? → 2.0b1+
Note: This is true for any access to nsIPromptService from C++ in the child process.

Updated

8 years ago
Assignee: nobody → doug.turner
(Assignee)

Updated

8 years ago
Assignee: doug.turner → azakai
(Assignee)

Comment 5

8 years ago
Created attachment 467915 [details] [diff] [review]
Part 1: Remote prompts in PromptService
Attachment #467915 - Flags: review?(mark.finkle)
(Assignee)

Comment 6

8 years ago
Created attachment 467916 [details] [diff] [review]
Part 2: Undo previous remoting code from bug 573635
Attachment #467916 - Flags: review?(mark.finkle)
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.
(Assignee)

Comment 8

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

Comment 9

8 years ago
Created attachment 469678 [details] [diff] [review]
Part 1 v2

Patch updated to use bug 585173, which looks like it is close to landing.
Attachment #467915 - Attachment is obsolete: true
Attachment #469678 - Flags: review?(mark.finkle)
Attachment #467915 - Flags: review?(mark.finkle)
(Assignee)

Updated

8 years ago
Depends on: 585173
Comment on attachment 469678 [details] [diff] [review]
Part 1 v2

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

Comment 11

8 years ago
(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.
no hacks, lets just fix 591052 asap.
Depends on: 591052
(Assignee)

Comment 13

8 years ago
Created attachment 470089 [details] [diff] [review]
Part 1 v3

Ok, updated patch using bug 591052, and moved entirely to PromptService. No hacks, all clean.
Attachment #469678 - Attachment is obsolete: true
Attachment #470089 - Flags: review?(mark.finkle)
Attachment #469678 - Flags: review?(mark.finkle)
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
Attachment #470089 - Flags: review?(mark.finkle) → review-
(Assignee)

Comment 15

8 years ago
Created attachment 471941 [details] [diff] [review]
Part 1 v4

Updated patch following comments and irc discussion (mainly better comments). Also mention temporary workaround with wrappedJSObject.
Attachment #470089 - Attachment is obsolete: true
Attachment #471941 - Flags: review?(mark.finkle)
(Assignee)

Comment 16

8 years ago
Created attachment 471943 [details] [diff] [review]
Part 2 v2

Rebased patch for undoing old remoting code from bug 573635.
Attachment #467916 - Attachment is obsolete: true
Attachment #471943 - Flags: review?(mark.finkle)
Attachment #467916 - Flags: review?(mark.finkle)
Attachment #471941 - Flags: review?(mark.finkle) → review+
Attachment #471943 - Flags: review?(mark.finkle) → review+
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
http://hg.mozilla.org/mobile-browser/rev/e305f7103d0a
http://hg.mozilla.org/mobile-browser/rev/cf490bf0fa1a
Status: REOPENED → RESOLVED
Last Resolved: 8 years ago8 years ago
Resolution: --- → FIXED
Flags: in-testsuite?
from the looks of things, no longer c-n
Keywords: checkin-needed
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.