Closed Bug 584401 Opened 9 years ago Closed 9 years ago

Prompts are not accessible to child process C++

Categories

(Firefox for Android Graveyard :: General, defect)

Other
Linux
defect
Not set

Tracking

(fennec2.0b1+)

VERIFIED FIXED
Tracking Status
fennec 2.0b1+ ---

People

(Reporter: starkov.egor, Assigned: azakai)

References

Details

Attachments

(2 files, 4 obsolete files)

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.
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
Closed: 9 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.
Assignee: nobody → doug.turner
Assignee: doug.turner → azakai
Attachment #467915 - Flags: review?(mark.finkle)
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.
(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.
Attached patch Part 1 v2 (obsolete) — Splinter Review
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)
Depends on: 585173
Comment on attachment 469678 [details] [diff] [review]
Part 1 v2

Can't you move the PromptServiceRemoter into the PromptService,js component?
(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
Attached patch Part 1 v3 (obsolete) — Splinter Review
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-
Attached patch Part 1 v4Splinter Review
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)
Attached patch Part 2 v2Splinter Review
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+
Keywords: checkin-needed
http://hg.mozilla.org/mobile-browser/rev/e305f7103d0a
http://hg.mozilla.org/mobile-browser/rev/cf490bf0fa1a
Status: REOPENED → RESOLVED
Closed: 9 years ago9 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.