899648 - Electrolysis: Make alert/prompt etc. work

Status: VERIFIED FIXED

(Toolkit :: General, defect)

Description

[Tom S [:evilpie]]

Do we need to proxy those into the chrome process?

Comment 1

[Mark Hammond [:markh] [:mhammond]]

FTR, note that in bug 875157, we actually had to take steps to *prevent* http auth etc dialogs from working in an e10s process (we wanted to avoid it as the browser was hidden, but it does demonstrate that in at least some cases, prompts worked fine)

Comment 2

[Tom S [:evilpie]]

Attachment: WIP

So I've got this working reasonable well. I am not sure if we need to support non-modal windows, but that seems definitely harder. I also had to disable the check that Mark mentioned in comment 1. We will have to find an other way to check for this condition.

Comment 3

[Tom S [:evilpie]]

Comment on attachment 785120 [details] [diff] [review]
WIP

At this stage feedback is probably more appropriate :)

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 785120 [details] [diff] [review]
WIP

>diff --git a/browser/base/content/browser-parent.js b/browser/base/content/browser-parent.js

>+  openPrompt: function(json, browser) {

>+        // tab-modal prompts need to watch for navigation changes, give it the
>+        // domWindow to watch for pagehide events.
>+        args.domWindow = null;

Either this code or comment seems wrong.

>diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml

>-                let tab = self._getTabForContentWindow(browser.contentWindow);
>+                //let tab = self._getTabForContentWindow(browser.contentWindow); ??
>+                let tab = self._getTabForBrowser(browser);

Why "??"?

>diff --git a/dom/interfaces/base/nsITabChild.idl b/dom/interfaces/base/nsITabChild.idl

I don't understand this change.

>diff --git a/toolkit/components/prompts/content/tabprompts.xml b/toolkit/components/prompts/content/tabprompts.xml

>+                if (this.args.domWindow)
>+                    this.args.domWindow.addEventListener("pagehide", this, false);

I don't like splitting the logic behind who gets to observe pagehide for e10s vs. non-e10s prompting. Is there a way to make this work the same in both cases?

>diff --git a/toolkit/components/prompts/src/nsPrompter.js b/toolkit/components/prompts/src/nsPrompter.js

>+        // Todo: not sure which check to use here.
>+        //if (winUtils && !winUtils.isParentWindowMainWidgetVisible) {
>+        //    throw Components.Exception("Cannot call openModalWindow on a hidden window",
>+        //                               Cr.NS_ERROR_NOT_AVAILABLE);
>+        //}

Why did you need to disable this?

>+            if (Services.appinfo.processType == 2) {
>+                openRemoteTabPrompt(this.domWin, args);

Services.appinfo.PROCESS_TYPE_CONTENT, but similarly, I don't like forking the logic.

>+        // Todo: Non modal windows won't work. Is that acceptable?

Acceptable for what? If this is some temporary hack, I guess so, but it's certainly not acceptable long-term. I'm worried we're accumulating too many hacks here.

Comment 5

[Tom S [:evilpie]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #4)
> Comment on attachment 785120 [details] [diff] [review]
> WIP
> 
> >diff --git a/browser/base/content/browser-parent.js b/browser/base/content/browser-parent.js
> 
> >+  openPrompt: function(json, browser) {
> 
> >+        // tab-modal prompts need to watch for navigation changes, give it the
> >+        // domWindow to watch for pagehide events.
> >+        args.domWindow = null;
> 
> Either this code or comment seems wrong.
I will just remove the comment, this was just copy pasted.
> 
> >diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml
> 
> >-                let tab = self._getTabForContentWindow(browser.contentWindow);
> >+                //let tab = self._getTabForContentWindow(browser.contentWindow); ??
> >+                let tab = self._getTabForBrowser(browser);
> 
> Why "??"?
> 
I wasn't sure why you would ever use the previous way, maybe just an oversight and nothing to worry about.
> >diff --git a/dom/interfaces/base/nsITabChild.idl b/dom/interfaces/base/nsITabChild.idl
> 
> I don't understand this change.
> 
I had to make the interface scriptable so that I can access the messageManager from JS. (Also forgot to rev the uuid). Maybe somebody else should review this change.
> >diff --git a/toolkit/components/prompts/content/tabprompts.xml b/toolkit/components/prompts/content/tabprompts.xml
> 
> >+                if (this.args.domWindow)
> >+                    this.args.domWindow.addEventListener("pagehide", this, false);
> 
> I don't like splitting the logic behind who gets to observe pagehide for
> e10s vs. non-e10s prompting. Is there a way to make this work the same in
> both cases?
> 
Agreed, I will think about a solution.
> >diff --git a/toolkit/components/prompts/src/nsPrompter.js b/toolkit/components/prompts/src/nsPrompter.js
> 
> >+        // Todo: not sure which check to use here.
> >+        //if (winUtils && !winUtils.isParentWindowMainWidgetVisible) {
> >+        //    throw Components.Exception("Cannot call openModalWindow on a hidden window",
> >+        //                               Cr.NS_ERROR_NOT_AVAILABLE);
> >+        //}
> 
> Why did you need to disable this?
> 
Because isParentWindowMainWidgetVisible always throws in the content process, thus we would never show anything. I need to find a way that implements the same check in an e10s friendly way.
> >+            if (Services.appinfo.processType == 2) {
> >+                openRemoteTabPrompt(this.domWin, args);
> 
> Services.appinfo.PROCESS_TYPE_CONTENT, but similarly, I don't like forking
> the logic.
> 
> >+        // Todo: Non modal windows won't work. Is that acceptable?
> 
> Acceptable for what? If this is some temporary hack, I guess so, but it's
> certainly not acceptable long-term. I'm worried we're accumulating too many
> hacks here.
Actually now that we could in theory totally block the content process like a "normal"/non-tab alert would, maybe we don't even need to respect "allowTabModal" anymore.
Of course this is just a hack :(

Comment 6

[Tom S [:evilpie]]

Attachment: v1

Addressed feedback. Implemented IsParentWindowMainWidgetVisible. I am not totally sure if it's equivalent to the code nsDOMWindowUtils::GetIsParentWindowMainWidgetVisible, maybe felipe knows this? The only remaining open quest is the case where we can't open modal windows.

Comment 7

[Justin Dolske [:Dolske]]

Comment on attachment 789040 [details] [diff] [review]
v1

Review of attachment 789040 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/prompts/src/nsPrompter.js
@@ +498,5 @@
> +    messageManager.sendAsyncMessage("Prompt:Open", args, {});
> +
> +    let thread = Services.tm.currentThread;
> +    while (!closed)
> +        thread.processNextEvent(true);

It would sure be preferable to avoid duplicating openTabPrompt(), since this can be touchy code.

Presumably the child process can't avoid spinning the even loop (unless some of the messageManager stuff will do that for the child implicitly?). But we should be thinking about making the parent process be able to show an asynch prompt (ie, not spin the event loops), since that's pretty hacky.

Comment 8

[Tom S [:evilpie]]

I am not sure how to avoid this duplication, unless we would start using the message manager for everything. The code is quite different in some ways and I already tried to simplify it.

Opening the tab prompt only does the processNextEvent stuff in the content, for chrome this should be totally asynchronous. (Unless this is something done by the tabprompt itself as well?) This actually makes this a nice win.

Comment 9

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 789040 [details] [diff] [review]
v1

Review of attachment 789040 [details] [diff] [review]:
-----------------------------------------------------------------

drive-by feedback (without bothering to duplicate the other feedback already added).

It's a real issue that tests aren't working when browser.tabs.remote=true - I think I should open a bug for that and have a go at sanely fixing it.

::: dom/ipc/TabParent.cpp
@@ +1085,5 @@
>  }
>  
>  bool
> +TabParent::RecvIsParentWindowMainWidgetVisible(bool* aIsVisible)
> +{

this looks like it is quite different to nsDOMWindowUtils::GetIsParentWindowMainWidgetVisible.  Seeing this is in the parent, I wonder if we can't just use something like:

  nsCOMPtr<nsIDOMWindowUtils> windowUtils;
  nsCOMPtr<nsIContent> frame = do_QueryInterface(mFrameElement);
  windowUtils = do_QueryInterface(frame->OwnerDoc()->GetWindow());
  return window->GetIsParentWindowMainWidgetVisible(aIsVisible);
?

::: toolkit/components/prompts/content/tabprompts.xml
@@ +166,5 @@
>                  this.Dialog.onLoad(null);
>  
>                  // Display the tabprompt title that shows the prompt origin when
>                  // the prompt origin is not the same as that of the top window.
> +                if (!args.isTopLevel)

I think isTopLevel should be named something else - it might be confused with it being like nsIWebProgress.isTopLevel.  Maybe isFromTopOrigin?

::: toolkit/components/prompts/src/nsPrompter.js
@@ +412,5 @@
>          if (!newPrompt && !forceCleanup)
>              return;
>          callbackInvoked = true;
>          if (newPrompt)
>              tabPrompt.removePrompt(newPrompt);

should newPrompt be set to null here and the pagehide listener removed?

@@ +419,5 @@
>  
>          PromptUtils.fireDialogEvent(domWin, "DOMModalDialogClosed");
>      }
>  
> +    domWin.addEventListener("pagehide", pagehide, false);

isn't this listener going to remain alive longer than it should?  Eg, onButtonClick in tabprompts.xml calls shutdownPrompt which previously removed the pagehide listener, but it looks like that case wouldn't cause this new listener to be removed.

Comment 10

[Tom S [:evilpie]]

Comment on attachment 789040 [details] [diff] [review]
v1

Review of attachment 789040 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/ipc/TabParent.cpp
@@ +1085,5 @@
>  }
>  
>  bool
> +TabParent::RecvIsParentWindowMainWidgetVisible(bool* aIsVisible)
> +{

Good point, I didn't try really hard here.

::: toolkit/components/prompts/content/tabprompts.xml
@@ +166,5 @@
>                  this.Dialog.onLoad(null);
>  
>                  // Display the tabprompt title that shows the prompt origin when
>                  // the prompt origin is not the same as that of the top window.
> +                if (!args.isTopLevel)

Ok, maybe hasTopPrincipal?

::: toolkit/components/prompts/src/nsPrompter.js
@@ +419,5 @@
>  
>          PromptUtils.fireDialogEvent(domWin, "DOMModalDialogClosed");
>      }
>  
> +    domWin.addEventListener("pagehide", pagehide, false);

This the same as above?

Comment 11

[Tom S [:evilpie]]

Untaking.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

(The stuff from Bug 548847 used to work, but apparently got broken at some point when promptservice was removed.)

Comment 13

[Chris Peterson [:cpeterson]]

Blake: at our last e10s meeting, Bill suggested that you take a look at this prompt bug. :)

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

I actually had a tab open where I'd already clicked "take" on this very bug!

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: evilpie's initial patch, merged

Justin, the problem with merging openTabPrompt and openRemoteTabPrompt is that the common logic is split amongst callbacks that actually do differ, so refactoring is going to be challenging at best.

I have a few extra cleanups in a separate patch that I'll attach here.

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Address some remaining review comments.

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Try to make non-tab-modal prompts work from child processes

With this patch, the only problem I know about is that background tabs don't pop to the foreground when an alert happens. I don't know how important that is from a security perspective. If we're using tab-modal dialogs, the only risk is the user missing a notification. For window-modal alerts, there's a chance that the user will be confused about which site is doing the alert. I will continue to try to debug that (I also realized that I haven't tested showModalDialog yet, I will do that presently).

Comment 18

[Justin Dolske [:Dolske]]

(In reply to Blake Kaplan (:mrbkap) from comment #17)
> Created attachment 8385058 [details] [diff] [review]
> Try to make non-tab-modal prompts work from child processes
> 
> With this patch, the only problem I know about is that background tabs don't
> pop to the foreground when an alert happens. I don't know how important that
> is from a security perspective.

That's driven by the DOMWillOpenModalDialog event, so presumably that needs to be adapted for E10S. (See also DOMModalDialogClosed)

Comment 19

[Justin Dolske [:Dolske]]

Comment on attachment 8374397 [details] [diff] [review]
evilpie's initial patch, merged

Review of attachment 8374397 [details] [diff] [review]:
-----------------------------------------------------------------

A few questions here, but assuming those are all OK I think this is r+.

::: browser/components/nsBrowserGlue.js
@@ +486,2 @@
>        ContentClick.init();
> +      RemotePrompt.init();

All this does is add a messageListener. Do we not yet have a common place to add these, to avoid pulling in the whole JSM on startup? Metro does, we should start doing the same.

In fact, once that's done, RemotePrompt.jsm just becomes empty except for openPrompt(). Are we likely to add more to it? If not, probably simpler to just roll openPrompt() into some existing place. EG, just adding it to nsBrowserGlue would seem fine. (Or nsPrompter, but that means adding some xpcom goop, so meh.)

::: browser/modules/RemotePrompt.jsm
@@ +45,5 @@
> +    browser.messageManager.addMessageListener("Prompt:ForceClose", function listener(message) {
> +      browser.messageManager.removeMessageListener("Prompt:ForceClose", listener);
> +
> +      if (newPrompt)
> +        newPrompt.abortPrompt();

Hmm, don't you need to ensure that the |newPrompt| here is the prompt the message is intended for? (Ie, not some other prompt in the window)

@@ +50,5 @@
> +    });
> +
> +    try {
> +      // We can't send a domWindow.
> +      args.domWindow = null;

I started to take a look at if we _really_ need a DOM window here (ie, if we can use a |browser| instead), but... ew. There's a bunch of goop in window watcher's openWindow that's fiddling with it. It's probably possible, since I think this is highly generic code that's trying to see if the specified window is a chrome window or not, and so for cases like prompts we could just skip all that. I think all we really need the window for is to get the parenting correct for the window hierarchy, and I assume that ends up using the browser's chrome (native) window anyway.

Seems like that would get us old-school windowed modal prompts for free (because this could just call into nsPrompter's openPrompt()), but I dunno if it's worth the extra effort. Although it would mean that E10S prompts would work exactly the same as non-E10S prompts...

::: toolkit/components/prompts/src/nsPrompter.js
@@ +419,5 @@
>  
>          PromptUtils.fireDialogEvent(domWin, "DOMModalDialogClosed");
>      }
>  
> +    domWin.addEventListener("pagehide", pagehide, false);

Last arg is optional these days, feel free to omit it.

@@ +468,5 @@
> +                         .getInterface(Ci.nsIDOMWindowUtils);
> +    winUtils.enterModalState();
> +    let closed = false;
> +
> +    messageManager.addMessageListener("Prompt:Close", function listener(message) {

Is this messageManager unique to the DOMWindow, such that you know the message is for this prompt?

There may still be some bugs if a page can get multiple prompts open. I think we try to prevent that today, but it's not foolproof. But if this happens it's already a (rare) bug, and other than the UX being confusing I'm not sure it actually breaks anything.

@@ +488,5 @@
> +    domWin.addEventListener("pagehide", pagehide, false);
> +    function pagehide() {
> +        domWin.removeEventListener("pagehide", pagehide);
> +        messageManager.sendAsyncMessage("Prompt:ForceClose", {});
> +    }

Should the pagehide handler be setting closed=true, so that the processNextEvent loop here ends?

@@ +535,2 @@
>                  return;
> +            } else {

else-after-return? Either remove the else, or make it a common return after the if-else.

@@ +542,4 @@
>              }
>          }
>  
> +        // Todo: Non modal windows don't work in e10s.

Looks like the later patch here fixes this TODO, so I won't complain about it. :)

Comment 20

[Justin Dolske [:Dolske]]

Comment on attachment 8374398 [details] [diff] [review]
Address some remaining review comments.

Review of attachment 8374398 [details] [diff] [review]:
-----------------------------------------------------------------

I don't know the TabParent.cpp code, if that's non-obvious someone else should review it.

Comment 21

[Justin Dolske [:Dolske]]

Comment on attachment 8385058 [details] [diff] [review]
Try to make non-tab-modal prompts work from child processes

Review of attachment 8385058 [details] [diff] [review]:
-----------------------------------------------------------------

Seem ok, I guess.

Comment 22

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Justin Dolske [:Dolske] from comment #19)
> All this does is add a messageListener. Do we not yet have a common place to
> add these, to avoid pulling in the whole JSM on startup? Metro does, we
> should start doing the same.

I don't see any central place. I'll hold off on this.

> In fact, once that's done, RemotePrompt.jsm just becomes empty except for
> openPrompt(). Are we likely to add more to it? If not, probably simpler to
> just roll openPrompt() into some existing place. EG, just adding it to
> nsBrowserGlue would seem fine. (Or nsPrompter, but that means adding some
> xpcom goop, so meh.)

With non-tab-modal prompts, this now has two functions, I'd prefer to leave it as is.

> Hmm, don't you need to ensure that the |newPrompt| here is the prompt the
> message is intended for? (Ie, not some other prompt in the window)

The message manager is per tab, I believe. I can't figure out a way to cause this to happen, but preventative code to avoid this is pretty easy to write, so I've gone ahead and done so.

> @@ +488,5 @@
> Should the pagehide handler be setting closed=true, so that the
> processNextEvent loop here ends?

I think it makes more sense to wait for the resulting Close to come back from the parent.

Comment 23

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Combined patch

I think this addresses all of the comments. I combined my patches with evilpie's because otherwise it was going to be pretty hard to review this.

The one problem that I've found with this patch is that if I do |window.open("foo.html"); alert("bar");| and foo.html navigates its opener, things behave... strangely (and the prompt stuff doesn't quite do the right thing). I'm still debugging it, but I don't think that's severe enough to warrent holding this patch up.

Comment 24

[Blake Kaplan (:mrbkap) (inactive)]

Review ping?

Comment 25

[Blake Kaplan (:mrbkap) (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/db0495da89ba

Comment 26

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/db0495da89ba

Comment 28

[Paul Silaghi, QA [:pauly]]

Reproduced in nightly 2014-04-01, Win 7 x64.
Prompt/alert work now - verified fixed FF 31 RC 2.