Closed Bug 775464 Opened 12 years ago Closed 12 years ago

HTTP authentication causes b2g/gecko crash

Categories

(Firefox OS Graveyard :: General, defect)

Product:
Firefox OS Graveyard
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(blocking-basecamp:+)

Status:
RESOLVED FIXED
Project Flags:
blocking-basecamp +

People

(Reporter: alive, Assigned: kk1fff)

References

Details

(Whiteboard: [mentor=jlebar][lang=js][LOE:M][qa-][WebAPI:P2])

Attachments

(3 files, 21 obsolete files)

Test case
7.81 KB, patch
justin.lebar+bug
: review+
Details | Diff | Splinter Review
Part 1: Add TopFrameElement to nsILoadContext
11.16 KB, patch
kk1fff
: review+
Details | Diff | Splinter Review
Part 2: Implement asyncPromptAuth and promptAuth.
18.08 KB, patch
kk1fff
: review+
Details | Diff | Splinter Review 
STR

1.Open browser app
2.link to https://phonebook.mozilla.org/
3.Crash!

I think the other web app has authentication access should also reproduce this.
OS: Mac OS X → All
Hardware: x86 → All
blocking-basecamp: --- → ?
blocking-basecamp: ? → +
Summary: Basic Authentication would cause b2g/gecko crash → HTTP authentication causes b2g/gecko crash
I've tried to trace the root cause, and I think the segmentation fault happens because when Gecko received an HTTP auth response header, it failed to create a new surface to display the auth dialog.
I did some simple tests. If Gecko gets a surface, it shows an auth dialog as we usually see on PC firefox. I am not sure if this dialog is what we expect, because the auth dialog is not in Gaia's look-and-feel.
(In reply to Patrick Wang from comment #1)
> I've tried to trace the root cause, and I think the segmentation fault
> happens because when Gecko received an HTTP auth response header, it failed
> to create a new surface to display the auth dialog.
> I did some simple tests. If Gecko gets a surface, it shows an auth dialog as
> we usually see on PC firefox. I am not sure if this dialog is what we
> expect, because the auth dialog is not in Gaia's look-and-feel.

The root cause is well-understood.  We override some prompts (e.g. window.alert) but not others (HTTP auth).  If we don't override the HTTP auth prompt, Gecko will try to load the prompt itself, causing this crash.

If you're interested in fixing the bug, the relevant code is in dom/browser-element/BrowserElementPromptService.jsm.  You'd fire a modal prompt to the browser element, basically just like window.prompt().
Adding reference to gaia issue: https://github.com/mozilla-b2g/gaia/issues/2611
Whiteboard: [mentor=jlebar][lang=js]
Patrick, could you take this bug with Justin as mentor?  Thank you!
Assignee: nobody → pwang
Hi Justin,

I am trying to get this bug fixed by implementing promptUsernameAndPassword. As a simple try, I added Ci.nsIAuthPrompt, Ci.nsIAuthPrompt2 into BrowserElementPrompt's QueryInterface, removed the exception thrown in promptUsernameAndPassword() in BrowserElementPromptService.jsm and let this function simply returns a "true". However, it still crashes when it tries to create a prompt for user to type username and password.

On the other hand, I removed the implementation in alert() function in BrowserElementPromptService.jsm and let the function throws an exception that promptUsernameAndPassword() throws. The alert message is no longer shown, but it won't crash when it runs a script that fires an alert message.

I am now look at nsPrompts.js, where I think decision of using native window is made. Could you give me some advise about this?

Thank you.
Patrick
> As a simple try, I added Ci.nsIAuthPrompt, Ci.nsIAuthPrompt2 into BrowserElementPrompt's 
> QueryInterface

Ah, I didn't know about these interfaces.  Okay...

> removed the exception thrown in promptUsernameAndPassword() in BrowserElementPromptService.jsm and 
> let this function simply returns a "true".

Well, that method is on nsIPrompt, not nsIAuthPrompt.  What about the methods on nsIAuthPrompt and nsIAuthPrompt2?  Is promptUsernameAndPassword being called before we crash?
> I am now look at nsPrompts.js, where I think decision of using native window is made. Could you give 
> me some advise about this?

There is no file nsPrompts.js in my tree.  Do you mean nsPrompter.js?

I think you're right to implement nsIAuthPrompt and nsIAuthPrompt2.  It looks like HTTP auth calls nsIAuthPrompt2::asyncPromptAuth.
See also how BrowserElementPromptFactory falls back to the original prompt service (in getPrompt).  You'll need to change the fallback conditions if you want to respond to nsIAuthPrompt requests.  (You might as well just remove the fallback entirely, since it's caused nothing but problems.)
I've tried to removed the fallback condition, and it works! After removed the if-statement and added nsIAuthPrompt, nsIAuthPrompt2 into QueryInterface of BrowserElementPrompt, asyncPromptAuth() is called and system will not crash.

It seems nsAuthPrompt2::asyncPromptAuth() is called first. If caller fail to find the interface, it calls nsAuthPrompt::promptUsernameAndPassword(). I think the fallback statement should just exam the argument and throw an exception if argument is invalid, instead of falling back and crashing system.

I plan to send a WIP that just updates the fallback condition and adds functions of nsAuthPrompt2, it should prevent system from crashing, but prompt may still not be shown. I should fire an event like prompt() does, but I haven't figure out what kind of data should be sent with the event, I didn't find the code that consumes the event in Gaia yet..
Hi patrick,

First of all, this bug is reported by a gaia dev, me, and I will be glad to continue the work in gaia side if you completed the gecko-side implementation.

I know little in gecko, but I had implemented similiar dialog UI in gaia before.
It is, window.alert/window.prompt/window.confirm.
You could reference them in
1. gaia layer: 
https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/modal_dialog.js
2. gecko bug
https://bugzilla.mozilla.org/show_bug.cgi?id=741587

If I am wrong, platform folks please correct me, but I think they are similiar stuff.
Hi Alive,

It seems prompt for authentication haven't been implemented in Gaia side. I think it will be better if UI is implemented by Gaia developer. And we may discuss what kind of data should be included in the event that is passed to Gaia.
IMO,
accept:
1. authentication type: basic or digest
2. the message string passed in the http header, indicating what to show in the dialog.
3. etc?

return:
a. basic auth: a base64-encryted string?
b. digest auth: TBD...I just forgot how to format a digest string, little complex I think.
Attached patch WIP (obsolete) — — Splinter Review 
WIP, change fallback to exception and add basic implementations of nsIAuthPrompt2
Attachment #646488 - Flags: feedback?(justin.lebar+bug)
From a Gecko (Necko) point of view, I would be interested in the stack trace.  Is the app crashing in native code of Gecko platform?  If so, then it should also be fixed (by me, probably, in a different bug) since just not providing a correct interfaces must not cause native code to crash.
Comment on attachment 646488 [details] [diff] [review]
WIP

You'd of course want to change the debug() messages where we're no longer falling back to the original prompt service.
Attachment #646488 - Flags: feedback?(justin.lebar+bug) → feedback+
(In reply to Honza Bambas (:mayhemer) from comment #14)
> From a Gecko (Necko) point of view, I would be interested in the stack
> trace.  Is the app crashing in native code of Gecko platform?  If so, then
> it should also be fixed (by me, probably, in a different bug) since just not
> providing a correct interfaces must not cause native code to crash.

This bug seems not make my Linux build of Firefox crash. It seems that nsBaseWidget::CreateCompositor() isn't called when Gecko creates an authentication prompt. nsBaseWidget::CreateCompositor() is the function that makes B2G/Gecko crash, because is fails to create a new surface from a global native window.
(In reply to Justin Lebar [:jlebar] from comment #15)
> Comment on attachment 646488 [details] [diff] [review]
> WIP
> 
> You'd of course want to change the debug() messages where we're no longer
> falling back to the original prompt service.

Oops, I forgot them. I will change them in later version. :)
In Android's PromptService.js, promptAuth() checks a property "signon.autologin.proxy"[1] to decide if auto login is used. Is there a similar property in B2G that we should check before we try to login automatically?

[1] https://mxr.mozilla.org/mozilla-central/source/mobile/android/components/PromptService.js#365
It's a gecko pref, so it's also available for b2g. Note that it default to false and is not set anywhere from a quick look at https://mxr.mozilla.org/mozilla-central/search?string=signon.autologin.proxy
Attached patch WIP v2 (obsolete) — — Splinter Review 
This wip includes implementation asyncPromptAuth and related functions.
Information of Http authentication is passed to Gaia in following structure.
{
  promptType:     "usernameandpassword",
  title:          title,
  text:           text,           // ex. "Enter password for xxx on aaa.com", might be null.
  checkMsg:       checkMsg,       // ex. "Remember password?"
  username:       username.value,
  password:       password.value,
  checkState:     checkState,     // Is "remember password" checked?
  host:           host,
  realm:          realm,
  returnValue:    null
}

expected return format from Gaia:
{
  username,
  password,
  remember,     // should the password be remembered?
  ok            // false for cancel.
}
Attachment #646488 - Attachment is obsolete: true
Attachment #647496 - Flags: feedback?(justin.lebar+bug)
Please comment in the original Github issue when this is fixed: https://github.com/mozilla-b2g/gaia/issues/2969
What I would like is for one or both of us to understand why each line of code we've copied needs to be there.  Ideally, we'd have tests for each bit of functionality we're adding.

I haven't had a chance yet to understand this dump of code, to see whether it's all actually necessary, but I hope I'll get to that tomorrow.  In the meantime, the interface in comment 20 looks sane.

Please let me know if there's other, specific feedback that you're blocked on.
(In reply to Justin Lebar [:jlebar] from comment #22)
> What I would like is for one or both of us to understand why each line of
> code we've copied needs to be there.  Ideally, we'd have tests for each bit
> of functionality we're adding.
> 
> I haven't had a chance yet to understand this dump of code, to see whether
> it's all actually necessary, but I hope I'll get to that tomorrow.  In the
> meantime, the interface in comment 20 looks sane.
> 
> Please let me know if there's other, specific feedback that you're blocked
> on.

Thank you for reviewing my work. I'm very new to Mozilla, hope I can fix this bug well.
Comment on attachment 647496 [details] [diff] [review]
WIP v2

It's customary to post patches with 8 lines of context (the default is 4).  You can add

  [diff]
  git = 1
  showfunc = 1
  unified = 8

to your ~/.hgrc to get the correct settings.

>diff --git a/dom/browser-element/BrowserElementChild.js b/dom/browser-element/BrowserElementChild.js
>--- a/dom/browser-element/BrowserElementChild.js
>+++ b/dom/browser-element/BrowserElementChild.js
>@@ -188,7 +188,9 @@
>     let returnValue = this._waitForResult(win);
> 
>     if (args.promptType == 'prompt' ||
>-        args.promptType == 'confirm') {
>+        args.promptType == 'confirm' ||
>+        args.promptType == 'usernameandpassword' ||
>+        args.promptType == 'password') {
>       return returnValue;
>     }
>   },
>diff --git a/dom/browser-element/BrowserElementPromptService.jsm b/dom/browser-element/BrowserElementPromptService.jsm
>--- a/dom/browser-element/BrowserElementPromptService.jsm
>+++ b/dom/browser-element/BrowserElementPromptService.jsm
>@@ -13,7 +13,10 @@
> 
> let EXPORTED_SYMBOLS = ["BrowserElementPromptService"];
> 
>+let global = this;
>+
> Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>+Cu.import("resource://gre/modules/Services.jsm");
> 
> function debug(msg) {
>   //dump("BrowserElementPromptService - " + msg + "\n");
>@@ -24,8 +27,208 @@
>   this._browserElementChild = browserElementChild;
> }
> 
>+//
>+// Copied from /mobile/android/components/PromptService.js, Removed UI-related
>+// methods and Android-specified methods: sendMessageToJava, fireDialogEvent,
>+// makeDialogText, getLocaleString and cleanUpLabel.
>+//
>+let PromptUtils = {
>+  get pwmgr() {
>+    delete this.pwmgr;
>+    return this.pwmgr = Cc["@mozilla.org/login-manager;1"].getService(Ci.nsILoginManager);
>+  },

Gaia keeps track of browsing history -- we have code in Gecko to do this,
called Places, but it's disabled in B2G -- so I think Gaia should similarly be
the one to keep track of login information.  That means we get to rip out a lot
of this code.

We might, in fact, be able to rip out all of PromptUtils.

>+  // JS port of http://mxr.mozilla.org/mozilla-central/source/embedding/components/windowwatcher/public/nsPromptUtils.h#89
>+  getAuthHostPort: function pu_getAuthHostPort(aChannel, aAuthInfo) {

Please don't reference line numbers in the code; the lines will just get out of
date.  You can reference the function in nsPromptUtils by name...  Or you can
just leave off the comment.  Anyway, you may not need this function.  :)

>+  getAuthTarget : function pu_getAuthTarget(aChannel, aAuthInfo) {
>+    let hostname, realm;
>+    // If our proxy is demanding authentication, don't use the
>+    // channel's actual destination.
>+    if (aAuthInfo.flags & Ci.nsIAuthInformation.AUTH_PROXY) {
>+        if (!(aChannel instanceof Ci.nsIProxiedChannel))
>+          throw "proxy auth needs nsIProxiedChannel";
>+
>+      let info = aChannel.proxyInfo;
>+      if (!info)
>+        throw "proxy auth needs nsIProxyInfo";
>+
>+      // Proxies don't have a scheme, but we'll use "moz-proxy://"
>+      // so that it's more obvious what the login is for.
>+      let idnService = Cc["@mozilla.org/network/idn-service;1"].getService(Ci.nsIIDNService);
>+      hostname = "moz-proxy://" + idnService.convertUTF8toACE(info.host) + ":" + info.port;
>+      realm = aAuthInfo.realm;
>+      if (!realm)
>+        realm = hostname;
>+
>+      return [hostname, realm];
>+    }
>+    hostname = this.getFormattedHostname(aChannel.URI);
>+
>+    // If a HTTP WWW-Authenticate header specified a realm, that value
>+    // will be available here. If it wasn't set or wasn't HTTP, we'll use
>+    // the formatted hostname instead.
>+    realm = aAuthInfo.realm;
>+    if (!realm)
>+      realm = hostname;
>+
>+    return [hostname, realm];
>+  },

I think you might need this function, but none of the others in PromptUtils.  But I'm not sure.

>@@ -81,6 +283,219 @@
>+
>+  /* ----------  nsIAuthPrompt2  ---------- */
>+
>+  _asyncPrompts: {},
>+  _asyncPromptInProgress: false,
>+
>+  // B2G's locale properties are in Gaia layer. We pass host, realm
>+  // to Gaia layer so message can be built in Gaia layer.

We call the layer above the "embedder", not "Gaia", because it's not always Gaia.  For example, any browser app might be filling the role of Gaia, because any browser app can contain <iframe mozbrowser>.

Similarly, we try not to refer to B2G, because this code is not B2G-specific.  For example, mozbrowser is used in some of our social-web code for sandboxing content.

>+  asyncPromptAuth: function asyncPromptAuth(aChannel, aCallback, aContext, aLevel, aAuthInfo) {

In the desktop Firefox implementation
(toolkit/components/prompts/src/nsPrompter.js), asyncPromptAuth is never
called.  I don't know whether we need this or not, but we shouldn't implement
this if we don't need it.

>@@ -93,20 +508,23 @@
> 
>   getPrompt: function(win, iid) {
>     // Try to find a browserelementchild for the window.
>-    if (iid.number != Ci.nsIPrompt.number) {
>-      debug("Falling back to wrapped prompt service because " +
>-            "we don't recognize the requested IID (" + iid + ", " +
>-            "nsIPrompt=" + Ci.nsIPrompt);
>-      return this._wrapped.getPrompt(win, iid);
>+    if (iid.number != Ci.nsIPrompt.number &&
>+        iid.number != Ci.nsIAuthPrompt.number &&
>+        iid.number != Ci.nsIAuthPrompt2.number) {
>+      debug("We don't recognize the requested IID (" + iid + ", " +
>+            "allowed IID: " +
>+            "nsIPrompt=" + Ci.nsIPrompt + ", " +
>+            "nsIAuthPrompt=" + Ci.nsIAuthPrompt + ", " +
>+            "nsIAuthPrompt2=" + Ci.nsIAuthPrompt2);
>+      throw Cr.NS_ERROR_INVALID_ARG;

There's a bug in the original and new error message where we don't close the
parenthesis in the message; please fix that, if you don't mind!

The next steps, I think, are ripping out all this saved-password business
(which is responsible for a large portion of the complexity in this code), and
writing tests which exercise this code, so we know, for example, whether we
need to implement asyncPromptAuth.

Please keep the patches coming.  :)
Attachment #647496 - Flags: feedback?(justin.lebar+bug) → feedback+
(In reply to Justin Lebar [:jlebar] from comment #24)
> >+  asyncPromptAuth: function asyncPromptAuth(aChannel, aCallback, aContext, aLevel, aAuthInfo) {
> 
> In the desktop Firefox implementation
> (toolkit/components/prompts/src/nsPrompter.js), asyncPromptAuth is never
> called.  I don't know whether we need this or not, but we shouldn't implement
> this if we don't need it.

I inserted logs in asyncPromptAuth() and promptAuth() to see if they are called.
asyncPromptAuth() is called before promptAuth(). If asyncPromptAuth()
throws an exception, promptAuth() will be called. Because asyncPromptAuth() is
expected to be async, there is an additional function _doAsyncPrompt() to
implement its async behavior.

If we don't implement asyncPromptAuth(), promptAuth() will still be called. We may
also remove a large piece of code. I've done some test about this, it seems
still working.
Attached patch WIP (obsolete) — — Splinter Review 
Removed logic to store to password manager, but keep asyncPromptAuth() and _doAsyncPrompt().
Attachment #647496 - Attachment is obsolete: true
Attachment #649165 - Flags: feedback?(justin.lebar+bug)
The comments below are more like a review than a feedback.  So I guess you get f+.  :)

>Bug 775464: Implement asyncPromptAuth, firing event to gaia and waiting for response

Please don't refer to "Gaia" in the commit message, per comment 24.

>diff --git a/dom/browser-element/BrowserElementPromptService.jsm b/dom/browser-element/BrowserElementPromptService.jsm
>+// Functions that helps tp format host/realm. Copied from
>+// /mobile/android/components/PromptService.js.
>+let PromptUtils = {

I'd prefer if these helpers were added to the BrowserElementPrompt object.

> BrowserElementPrompt.prototype = {
>-  QueryInterface: XPCOMUtils.generateQI([Ci.nsIPrompt]),
>+  QueryInterface: XPCOMUtils.generateQI([Ci.nsIPrompt,
>+                                         Ci.nsIAuthPrompt,
>+                                         Ci.nsIAuthPrompt2]),
> 
>   alert: function(title, text) {
>     this._browserElementChild.showModalPrompt(
>       this._win, {promptType: "alert", title: title, message: text});
>   },
> 
>   alertCheck: function(title, text, checkMsg, checkState) {
>     // Treat this like a normal alert() call, ignoring the checkState.  The
>@@ -76,42 +152,213 @@ BrowserElementPrompt.prototype = {
> 
>   promptPassword: function(title, text, password, checkMsg, checkState) {
>     throw Cr.NS_ERROR_NOT_IMPLEMENTED;
>   },
> 
>   select: function(title, text, aCount, aSelectList, aOutSelection) {
>     throw Cr.NS_ERROR_NOT_IMPLEMENTED;
>   },
>+
>+  /* ----------  nsIAuthPrompt2  ---------- */
>+
>+  _asyncPrompts: {},
>+  _asyncPromptInProgress: false,
>+
>+  // B2G's locale properties are in embedder. We pass host, realm
>+  // to Gaia layer so message can be built in Gaia layer.

Please change "B2G" and "Gaia", per comment 24.

>+  _promptUsernameAndPassword: function _promptUsernameAndPassword(
>+    username, password, host, realm) {
>+
>+    // expecting return value from gaia.

Please change "Gaia", per comment 24.

>+    // {
>+    //   string username,
>+    //   string password,
>+    //   bool   ok            // false for cancel.
>+    // };
>+    let rv = this._browserElementChild.showModalPrompt(
>+      this._win,
>+      {
>+        promptType:     "usernameandpassword",
>+        host:           host,
>+        realm:          realm,
>+        returnValue:    null

I don't think you need returnValue.  Is it being used somewhere?

>+      });
>+    username.value = rv.username;
>+    password.value = rv.password;
>+
>+    return rv.ok;
>+  },
>+
>+  _promptPassword: function _promptPassword(password, host, realm) {
>+
>+    // expecting return value from gaia.

"Gaia", comment 24.

>+    // {
>+    //   string password,
>+    //   bool   ok            // false for cancel.
>+    // };
>+    let rv = this._browserElementChild.showModalPrompt(
>+      this._win,
>+      {
>+        promptType:     "password",
>+        host:           host,
>+        realm:          realm,
>+        returnValue:    null

Don't think you need returnValue.

>+      });
>+
>+    password.value = rv.password;
>+    return rv.ok;
>+  },
>+
>+  promptAuth: function promptAuth(aChannel, aLevel, aAuthInfo) {
>+    dump("In promptAuth\n");

Please remove this and other dump() calls in the final patch (or convert it to
a debug() call, implemented as in BrowserElementChild.js).

>+    let [hostname, realm] = PromptUtils.getAuthTarget(aChannel, aAuthInfo);
>+    let username = { value: "" };
>+    let password = { value: "" };
>+
>+    let ok = false;
>+    if (aAuthInfo.flags & Ci.nsIAuthInformation.ONLY_PASSWORD) {
>+      ok = this._promptPassword(password, hostname, realm);
>+    } else {
>+      ok = this._promptUsernameAndPassword(username, password, hostname, realm);
>+    }
>+
>+    PromptUtils.setAuthInfo(aAuthInfo, username.value, password.value);

setAuthInfo is only called here; can you just inline it?

>+  asyncPromptAuth: function asyncPromptAuth(aChannel, aCallback, aContext, aLevel, aAuthInfo) {
>+    let cancelable = null;
>+    try {

>+      // If the user submits a login but it fails, we need to remove the
>+      // notification bar that was displayed. Conveniently, the user will
>+      // be prompted for authentication again, which brings us here.
>+      //this._removeLoginNotifications();

Please remove this chunk of comments, since it's not relevant.

> BrowserElementPromptFactory.prototype = {
>   classID: Components.ID("{24f3d0cf-e417-4b85-9017-c9ecf8bb1299}"),
>   QueryInterface: XPCOMUtils.generateQI([Ci.nsIPromptFactory]),
> 
>   getPrompt: function(win, iid) {
>     // Try to find a browserelementchild for the window.
>-    if (iid.number != Ci.nsIPrompt.number) {
>-      debug("Falling back to wrapped prompt service because " +
>-            "we don't recognize the requested IID (" + iid + ", " +
>-            "nsIPrompt=" + Ci.nsIPrompt);
>-      return this._wrapped.getPrompt(win, iid);
>+    if (iid.number != Ci.nsIPrompt.number &&
>+        iid.number != Ci.nsIAuthPrompt.number &&
>+        iid.number != Ci.nsIAuthPrompt2.number) {
>+      debug("We don't recognize the requested IID (" + iid + ", " +
>+            "allowed IID: " +
>+            "nsIPrompt=" + Ci.nsIPrompt + ", " +
>+            "nsIAuthPrompt=" + Ci.nsIAuthPrompt + ", " +
>+            "nsIAuthPrompt2=" + Ci.nsIAuthPrompt2) + ")";

Looks like this line should be

>             "nsIAuthPrompt2=" + Ci.nsIAuthPrompt2 + ")");

As currently implemented, all auth prompts (both sync and async) are going through BrowserElementChild::showModalPrompt, which is the same code as implements alert().  That code is synchronous: If you think about how alert() works, it has to block script execution until the user presses OK, so here, we block script execution starting when the prompt is shown, and ending when the prompt window is closed.  That happens in both the "sync" and "async" cases, but that's not the behavior we want for the async case.

If we want to implement async prompts, we'll have to do so differently than is currently here.  The problem is, the async runnable calls promptAuth(), which itself is blocking (that is, not async).  We'd need to add a new method to BrowserElementChild which is not blocking.

I think the simplest way forward here is to only implement the synchronous prompt functionality for now, and see how far that gets us.  We can always come back and implement async prompts.  That is, unless you discover some big problem caused by not implementing async prompts.

Let me know if you have questions about the above; it's complicated, and I'm not sure I've explained it well.

If we go this route, the next step would be writing testcases for this bug.  You'll probably need to use an sjs file to do the HTTP auth.  We have some sjs files in dom/browser-element/mochitest which might be instructive, but feel free to ping me if you need assistance.
Attachment #649165 - Flags: feedback?(justin.lebar+bug) → feedback+
About event.detail.promptType,
I would propose to use the term `httpauthentication` instead of `usernameandpassword`, for semantic purpose.
How do you think?
> I would propose to use the term `httpauthentication` instead of `usernameandpassword`, for semantic 
> purpose.

I agree that would be better if usernameandpassword is used only for http auth.  But is that true?  usernameandpassword is probably also used for FTP auth (which we may or may not support at the moment).  I dunno if there's anything else.

If you could detect HTTP auth as different from the other kinds of auth, then I'd be OK maybe doing

  showModalPrompt({type: usernameandpassword, reason: httpauthentication})

or something like that.  Then you could have a "reason: other" for reasons you couldn't detect properly.  And then calling code which didn't care about whether it's httpauthentication wouldn't have to map http auth --> prompt for username and password.

As for how to detect that the prompt is an HTTP auth prompt...I guess you'd look at the channel?  I'm not quite sure...
This patch implements promptAuth. Event that sent to embedder includes following data
      {
        promptType:     "usernameandpassword",
        reason:         reason,
        host:           host,
        realm:          realm
      });
For authentication, reason will be "httpauthentication".
Attachment #649165 - Attachment is obsolete: true
Attachment #649998 - Flags: review?(justin.lebar+bug)
Comment on attachment 649998 [details] [diff] [review]
Patch: Implement promptAuth and fire event to embedder

I'll look at this, but I can't r+ it without tests.  Are you working on those?
Attachment #649998 - Flags: review?(justin.lebar+bug) → feedback?(justin.lebar+bug)
> +  promptAuth: function promptAuth(aChannel, aLevel, aAuthInfo) {
> ...
> +    if (aAuthInfo.flags & Ci.nsIAuthInformation.ONLY_PASSWORD) {
> +      ok = this._promptPassword(
> +        password, hostname, realm, "httpauthentication");
> +    } else {
> +      ok = this._promptUsernameAndPassword(
> +        username, password, hostname, realm, "httpauthentication");
> +    }

I don't think promptAuth is used only for HTTP authentication.  For example, it seems to be used for proxy auth.
Comment on attachment 649998 [details] [diff] [review]
Patch: Implement promptAuth and fire event to embedder

I don't know the ins and outs of this code, so if you want to provide this reason field to the embedder, you'll need to dig around or ask around to figure out the reasons that promptAuth is called and how you can tell them apart.

I'd start with Necko folks, such as Josh Aas (:josh), or Jason Duell (:jduell).
Attachment #649998 - Flags: feedback?(justin.lebar+bug) → feedback-
Honza, can you can provide feedback on when auth prompts are used (see comment 22 and 23) before you go on vacation?  Not sure anyone else knows this code as well as you do (Patrick, pinging biesi on IRC might also be a good bet)
Ah, I thought people already knew what to do in this bug.

I'll comment later today.
Searched promptAuth on mxr and tried to find out when promptAuth is called. It looks like ftp, proxy and http auth would call this method. I think we could tell them from nsIChannel and nsIAuthInformation arguments.
(In reply to Patrick Wang [:kk1fff] from comment #1)
> I've tried to trace the root cause, and I think the segmentation fault
> happens because when Gecko received an HTTP auth response header, it failed
> to create a new surface to display the auth dialog.

Do you remember if the crash was of the browser, or of the whole phone?

In the latter case (that's what it sounds like, based on what seemed to be crashing in the early comments of this bug), we're in some trouble here.  I'd been assuming that the promptAuth calls were happening in the child process, but if that's not true, we'll have to figure out how to map from the parameters passed to promptAuth to a BrowserElementParent.

IOW, this patch is going to change a lot, if we're right about this.  I'm sorry for leading you astray, Patrick!
I didn't check deeply the patch.

First, you only need to implement nsIAuthPrompt2.  For HTTP you only need the async method (promptAuth would be used when asyncPromptAuth failed, e.g. with NOT_IMPLEMENTED).  For FTP you also need the sync method.

To distinguish the principal (www or proxy auth) check flags on nsIAuthInformation (passed by the channel to both methods).

Prompts are invoked on the parent (chrome, root, main, not sure what all names it has) process.  Checked with e10s enabled fennec build.

Good example of implementation of nsIAuthPrompt2 is at [1] or [2].  [2] is the generic implementation of login manger with an auth cache that Firefox is using.

[1] http://hg.mozilla.org/mozilla-central/annotate/9249f0c9ca93/mobile/android/components/PromptService.js#l446
[2] http://hg.mozilla.org/mozilla-central/annotate/9249f0c9ca93/toolkit/components/passwordmgr/nsLoginManagerPrompter.js#l636
My plan is to add a method on nsILoadContext which lets you get the relevant frame element for the load.  Then we can use that to get the relevant mozbrowser.

This is perhaps getting outside good-first-bug territory, so I'm happy to write that patch.
I haven't tested this, but it should probably work.  :)
So here's what I think we need to do now:

* When we get a promptAuth call, we try to QI the channel's notificationCallbacks to nsILoadContext.  If that doesn't throw, we try to call nsILoadContext::GetTopFrameElement (which may throw also).

* If we don't get a frame element, promptAuth fails.

* If we do get a frame element, we have to map that to a BrowserElementParent.  This will work basically just like the current code to map from window --> BrowserElementParent, except you don't need the equivalent of the window.top call in the existing code.

* We'll have to write new prompt code in BrowserElementParent -- we can't use BrowserElementParent::ShowModalPrompt, because this isn't a sync prompt that the child is waiting on.

Does that make sense, Patrick?
> This will work basically just like the current code to map from window --> BrowserElementParent

Er, window --> BrowserElementChild.
Comment on attachment 650655 [details] [diff] [review]
Add TopFrameElement to nsILoadContext.

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

I suspect you're using a tree that's a day or two old?  The various *ParentChannel classes no longer implement nsILoadContext directly--it's now implemented by /docshell/base/LoadContext (see bug 776174).   The good news about this is that it will make it much easier to centralize this logic to all the channels--we just need to make LoadContext.cpp implement the new method.  The bad news is that we'll now need to have LoadContext know about TabParent (since it needs it to get the TopFrameElement).  Might make sense to just make the TabParent a member of IPC:LoadGroup.   Looks like we're going to need to pass TabParent for all channels anyway (right now it's just HTTP) to do security check on AppID anyway.   Feel free to give that a stab or I can roll patch for you when I get a sec.
Attachment #650655 - Flags: feedback-
> I suspect you're using a tree that's a day or two old?

Aww, man!

> Feel free to give that a stab or I can roll patch for you when I get a sec.

If you want to spin the patch sometime soon, that would be awesome.
(In reply to Justin Lebar [:jlebar] from comment #37)
> (In reply to Patrick Wang [:kk1fff] from comment #1)
> > I've tried to trace the root cause, and I think the segmentation fault
> > happens because when Gecko received an HTTP auth response header, it failed
> > to create a new surface to display the auth dialog.
> 
> Do you remember if the crash was of the browser, or of the whole phone?
> 

The crash is happened in b2g process.

#0  nsBaseWidget::CreateCompositor (this=0x82abe0)
    at /home/patrick/w/bug780087/mc/widget/xpwidgets/nsBaseWidget.cpp:902
#1  0x40b7c918 in nsWindow::GetLayerManager (this=0x82abe0, aShadowManager=<value optimized out>, 
    aBackendHint=<value optimized out>, aPersistence=<value optimized out>, aAllowRetaining=0x0)
    at /home/patrick/w/bug780087/mc/widget/gonk/nsWindow.cpp:539

In the 0th frame, nsBaseWidget is going to call NS_RUNTIMEABORT("failed to construct LayersChild").


#20 0x40f0c70e in js::RunScript (cx=0x1ce5e0, script=0x4693c5d8, fp=0x422bb258)
    at /home/patrick/w/bug780087/mc/js/src/jsinterp.cpp:302

the frame of nsPrompter.js is also in b2g process.

> In the latter case (that's what it sounds like, based on what seemed to be
> crashing in the early comments of this bug), we're in some trouble here. 
> I'd been assuming that the promptAuth calls were happening in the child
> process, but if that's not true, we'll have to figure out how to map from
> the parameters passed to promptAuth to a BrowserElementParent.
> 
> IOW, this patch is going to change a lot, if we're right about this.  I'm
> sorry for leading you astray, Patrick!

It's ok. It a very good chance for me to learn. :)
(In reply to Justin Lebar [:jlebar] from comment #41)
> So here's what I think we need to do now:
> 
> * When we get a promptAuth call, we try to QI the channel's
> notificationCallbacks to nsILoadContext.  If that doesn't throw, we try to
> call nsILoadContext::GetTopFrameElement (which may throw also).
> 
> * If we don't get a frame element, promptAuth fails.
> 
> * If we do get a frame element, we have to map that to a
> BrowserElementParent.  This will work basically just like the current code
> to map from window --> BrowserElementParent, except you don't need the
> equivalent of the window.top call in the existing code.
> 
> * We'll have to write new prompt code in BrowserElementParent -- we can't
> use BrowserElementParent::ShowModalPrompt, because this isn't a sync prompt
> that the child is waiting on.
> 
> Does that make sense, Patrick?

Sounds good. So what we need to do would be

1. Obtain the frame when BrowserElementChild is created, so it can insert an
   entity to the map from frame to itself.
2. Obtain the frame from nsIChannel when promptAuth is called, thus we can find
   the corresponding BrowserElementChild.
3. Send prompt message asynchronously to parent. And handle the message that
   received from parent when embedder gets the result.

Did I understand correctly?
> Did I understand correctly?

Not quite.

BrowserElementChild may live in the child process.  If so, the <iframe> element is not in that process -- you can't get a reference to that element.

So to tweak your instructions:

> 1. Obtain the frame when BrowserElement/Parent/ is created, so it can insert an
>    entity to the map from frame to itself.
> 2. Obtain the frame from nsIChannel when promptAuth is called, thus we can find
>    the corresponding BrowserElement/Parent/.
> 3. (not needed)
Asynchronous prompt in this patch is implemented by calling promptAuth() of BrowserElementParent. The function send embedder an event, which contains a callback function to pass username and password back.

I haven't implement the part that gets frame from nsIChannel. But I think if it is feasible to get frame from the window that passed to getPrompt, and use it to find the BrowserElementParent. it might be easier than we obtain the frame from nsIChannel.
Attachment #649998 - Attachment is obsolete: true
Attachment #651622 - Flags: feedback?(justin.lebar+bug)
>  I think if it is feasible to get frame from the window that passed to getPrompt, and use it to find 
> the BrowserElementParent. it might be easier than we obtain the frame from nsIChannel.

I'm not sure what you mean by "the window that passed to getPrompt()".

Do you mean, you want to get the window which caused the prompt to show?  e.g., if it's an HTTP auth prompt, the window which is loading the content protected by HTTP authentication?

That window lives in the child process (if the iframe is remote), so is inaccessible from BrowserElementParent in the OOP case.  You can only get the iframe which contains that window.  And the window triggering the HTTP auth may be nested within that iframe; that is, we might have

  <iframe mozbrowser>  <-- you can get this iframe element in the parent
    <iframe>
      <iframe src="http://protected.by.http.auth">  <-- you cannot get this iframe or its window in the parent

Does that make sense?
Pass tab parent to LoadContext when it is created by channel parent. And use topFrameElement to get the from that owns the channel.
In asyncPromptAuth, find browser element parent that corresponds to the nsIChannel. And use the browser element to send an async message to embedder.
Summary of this wip:
1. Update asyncPromptAuth. It sends 'showmodalprompt' message asynchronously
   through BrowserElementParent.
2. Implement "getReason" function, using the information provided from
   channel and authinfo. the reason will be "httpproxy", "httpauth" or "ftplogin".

There is still a problem: When the auth process is finished, asyncPromptAuth
calls callback with username/password information, but the browser didn't
load the page content, just change the URL in address bar.
Attachment #651622 - Attachment is obsolete: true
Attachment #652031 - Attachment is obsolete: true
Attachment #651622 - Flags: feedback?(justin.lebar+bug)
Attachment #652081 - Flags: feedback?(justin.lebar+bug)
Comment on attachment 652028 [details] [diff] [review]
Part 1: Add TopFrameElement to nsILoadContext

Nice.

> +    mLoadContext = new LoadContext(loadContext, mTabParent.get());

You shouldn't need .get().  (nsRefPtr<T> will implicitly cast itself to T*.)  On the other hand, you /do/ need .get() when doing static_cast<T*>(myRefPtr).
Attachment #652028 - Flags: feedback+
Comment on attachment 652028 [details] [diff] [review]
Part 1: Add TopFrameElement to nsILoadContext

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

::: docshell/base/nsILoadContext.idl
@@ +13,5 @@
>   * An nsILoadContext represents the context of a load.  This interface
>   * can be queried for various information about where the load is
>   * happening.
>   */
>  [scriptable, uuid(48b5bf16-e0c7-11e1-b28e-91726188709b)]

The UUID needs to change.
Comment on attachment 652081 [details] [diff] [review]
Part 2: Implement asyncPromptAuth and promptAuth.

This looks pretty good to me.

One thing that should be changed is that right now, we allow only one async prompt at a time, globally, across the whole phone.  (this._asyncPromptInProgress)  But this allows any app which contains <iframe mozbrowser> to potentially keep all other apps from showing prompts.

Instead, you should allow one prompt at a time per top frame element.


You're going to have to rebase this atop the changes in bug 781521 which landed last night.  Let me know if it's confusing.

I feel like once we have an sjs test, the failure to load the page that you're seeing should be easier to debug, because we'll know exactly which HTTP responses are or aren't being sent by the browser.

Otherwise, I'd recommend having a look with wireshark to see what's going on. (You might be able to get the equivalent information without wireshark by using [1].) But maybe jduell, who actually knows our networking stack, has a better idea.  :)

[1] https://developer.mozilla.org/en-US/docs/HTTP_Logging
Attachment #652081 - Flags: feedback?(justin.lebar+bug) → feedback+
Whiteboard: [mentor=jlebar][lang=js] → [mentor=jlebar][lang=js][LOE:L]
Whiteboard: [mentor=jlebar][lang=js][LOE:L] → [mentor=jlebar][lang=js][LOE:M]
Comment on attachment 652028 [details] [diff] [review]
Part 1: Add TopFrameElement to nsILoadContext

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

Consider this a +r on the necko part of the patch with jdm's nit and jlebar's fix in comment 53 fixed  (for some reason in this bug I can't seem to both +r and request an additional r?, so I'm just marking f+ but really I mean +r).

Smaug: fairly trivial change to mozilla::LoadContext.
Attachment #652028 - Flags: review?(bugs)
Attachment #652028 - Flags: feedback+
Attachment #650655 - Attachment is obsolete: true
Comment on attachment 652028 [details] [diff] [review]
Part 1: Add TopFrameElement to nsILoadContext

So, what keeps what alive here? Is it possible to get in to
LoadContext->mTabParent->LoadContext cycle?

>-  LoadContext(const IPC::SerializedLoadContext& toCopy)
>+    LoadContext(const IPC::SerializedLoadContext& toCopy)
>     : mIsNotNull(toCopy.mIsNotNull)
>     , mIsContent(toCopy.mIsContent)
>     , mUsePrivateBrowsing(toCopy.mUsePrivateBrowsing)
>     , mIsInBrowserElement(toCopy.mIsInBrowserElement)
>     , mAppId(toCopy.mAppId)
>+    , mTabParent(nullptr)
>+  {}
>+
>+  LoadContext(const IPC::SerializedLoadContext& toCopy, mozilla::dom::TabParent* tp)
>+    : mIsNotNull(toCopy.mIsNotNull)
>+    , mIsContent(toCopy.mIsContent)
>+    , mUsePrivateBrowsing(toCopy.mUsePrivateBrowsing)
>+    , mIsInBrowserElement(toCopy.mIsInBrowserElement)
>+    , mAppId(toCopy.mAppId)
>+    , mTabParent(tp)
>   {}
> 

Parameters should be in form aParameterName

> [scriptable, uuid(48b5bf16-e0c7-11e1-b28e-91726188709b)]
> interface nsILoadContext : nsISupports
update uuid when changing an interface
Comment on attachment 652028 [details] [diff] [review]
Part 1: Add TopFrameElement to nsILoadContext

 
> NS_IMETHODIMP
>+LoadContext::GetTopFrameElement(nsIDOMElement** aElement)
>+{
>+  *aElement = nullptr;
>+  if (!mTabParent) {
>+    return NS_OK;
>+  }
>+
>+  nsCOMPtr<nsIDOMElement> element = mTabParent->GetOwnerElement();
>+  element.forget(aElement);
>+  return NS_OK;
>+}

So, could the object hold nsWeakRef to the TabParent->GetOwnerElement();
That at least ensures no cycles are created.
Either do that, or proof that no cycles can be created.
Attachment #652028 - Flags: review?(bugs) → review-
I've updated the patch, summarized below:

1. Fix code that is mentioned in comment 53.
2. Use weak reference to hold reference of mTabParent, and let TabParent implement nsSupportsWeakRefernce.
3. Correct name of argument in LoadContext.h.
Attachment #652028 - Attachment is obsolete: true
Attachment #653695 - Flags: review?(bugs)
This patch includes some errors that are discovered when running test.

1. Code of falling back to old prompt, which had been removed in previous version, is added into patch of this version. Because after BrowserElementPromptService replaced the prompt factory, desktop version of firefox can no longer show a native prompt without fallback mechanism.

2. In OOP mode, when a BrowserElementPrompt is built for asyncPromptAuth, it cannot get reference of BrowserElementChild. Therefore, getPrompt  cannot tell if it should fallback by trying to get a reference of BrowserElementChild. Therefore, it returns an instance of AuthPromptWrapper, which wraps two implementations of asyncPromptAuth, and will decide which should be used with nsIChannel that passed to asyncPromptAuth().

3. Maintain asyncPromptInProgress per app. So an app can prompt when there is another app showing a prompt.

4. After user typed username and password, page can be loaded now.

There are still some problems in this update:

1. When an app is showing a prompt, other app cannot show prompt for the same site. For example, if there are two apps open https://need.auth/ at the same time, only one app will show the prompt, the other one shows "loading" (like network is very slow). I think it might be blocked by necko, because asyncPromptAuth is not called in this situation.

2. I am not sure if the behavior is correct in OOP mode. For browser app, which listens to mozbrowsershowmodalprompt as well, we have the structure

   [system app] --- [browser app] --- [browser content]

In in-process mode, mozbrowsershowmodalprompt is fired to browser app when browser is navigating a site that need auth. While in oop mode, system app handles the event. I think it might because system app and browser app are in different processes, and we can only get the reference of BrowserElementParent of browser app.

I think it would be better if we make asyncPromptAuth get called in the process that BrowserElementChild lives, and use the child to inform its parent to fire mozbrowser event, like other prompts, but I guess it will be a more complex task. As this bug can cause b2g crash and marked blocking basecamp, May be we can use current approach as a quick fix and fire another bug for future work, do it sound reasonable?

Thanks
Attachment #652081 - Attachment is obsolete: true
Attachment #653715 - Flags: review?(justin.lebar+bug)
Attached patch Test case (obsolete) — — Splinter Review 
This test case includes handling http 401, the http authentication. And proxy authentication, http 407 response.
Attachment #653716 - Flags: review?(justin.lebar+bug)
Comment on attachment 653695 [details] [diff] [review]
Part 1: Add TopFrameElement to nsILoadContext

Why weakref to tabparent, but not to the element?
Elements already support weakrefs
Attachment #653695 - Flags: review?(bugs) → review-
Keep weak reference of top element, instead of TabParent.
Attachment #653695 - Attachment is obsolete: true
Attachment #654130 - Flags: review?(bugs)
Comment on attachment 654130 [details] [diff] [review]
Part 1: Add TopFrameElement to nsILoadContext


> class LoadContext MOZ_FINAL : public nsILoadContext
> {
> public:
>   NS_DECL_ISUPPORTS
>   NS_DECL_NSILOADCONTEXT
> 
>-  LoadContext(const IPC::SerializedLoadContext& toCopy)
>-    : mIsNotNull(toCopy.mIsNotNull)
>-    , mIsContent(toCopy.mIsContent)
>-    , mUsePrivateBrowsing(toCopy.mUsePrivateBrowsing)
>-    , mIsInBrowserElement(toCopy.mIsInBrowserElement)
>-    , mAppId(toCopy.mAppId)
>+  LoadContext(const IPC::SerializedLoadContext& aToCopy)
>+    : mIsNotNull(aToCopy.mIsNotNull)
>+    , mIsContent(aToCopy.mIsContent)
>+    , mUsePrivateBrowsing(aToCopy.mUsePrivateBrowsing)
>+    , mIsInBrowserElement(aToCopy.mIsInBrowserElement)
>+    , mAppId(aToCopy.mAppId)
>+    , mTopFrameElement(nullptr)
Nit, no need to manually assign null to nsWeakPtr.

>diff --git a/netwerk/protocol/http/HttpChannelParent.cpp b/netwerk/protocol/http/HttpChannelParent.cpp
Someone else should review this. (Perhaps jduell did review this already)
Attachment #654130 - Flags: review?(bugs) → review+
Can you push this to try?  Let me know if you need me to.  I'd also be happy to vouch for L1 access, if you don't have it.
Comment on attachment 654130 [details] [diff] [review]
Part 1: Add TopFrameElement to nsILoadContext

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

+r on the necko bits
Attachment #654130 - Flags: review+
This test is really excellent.  Just a few changes and questions:

>+function testFail(msg) {
>+  ok(false, SpecialPowers.wrap(msg).json);
>+}

Can we do JSON.stringify(msg) instead, or is this somehow different?

>+function testProxyAuth(e) {

Nit: Could you move this function below testProxyStart, since it runs after
that one?

>+  iframe.removeEventListener("mozbrowsershowmodalprompt", testProxyAuth);
>+
>+  // Will authenticate with correct password, prompt should not be
>+  // called again.
>+  iframe.addEventListener("mozbrowsershowmodalprompt", testFail);
>+  iframe.addEventListener("mozbrowsertitlechange", function onTitleChange(e) {
>+    iframe.removeEventListener("mozbrowsertitlechange", onTitleChange);
>+    iframe.removeEventListener("mozbrowsershowmodalprompt", testFail);
>+    is(e.detail, 'auth');

Nit: Perhaps use a different title for the HTTP auth and proxy auth pages, so
we can tell them apart?

>+function testProxyStart() {
>+  iframe.addEventListener("mozbrowsershowmodalprompt", testProxyAuth);
>+
>+  var Ci = Components.interfaces;
>+  var pwmgr = SpecialPowers.wrap(Components)
>+    .classes["@mozilla.org/login-manager;1"]
>+    .getService(Ci.nsILoginManager);
>+
>+  function addLogin(host, realm, user, pass) {
>+    var login = SpecialPowers.wrap(Components)
>+      .classes["@mozilla.org/login-manager/loginInfo;1"]
>+      .createInstance(Ci.nsILoginInfo);
>+    login.init(host, null, realm, user, pass, "", "");
>+    pwmgr.addLogin(login);
>+
>+    // Clean the login information.
>+    finalizeFunctions.push(function() {
>+      pwmgr.removeLogin(login);
>+
>+      var authMgr = SpecialPowers.wrap(Components)
>+        .classes['@mozilla.org/network/http-auth-manager;1']
>+        .getService(Ci.nsIHttpAuthManager);
>+      authMgr.clearAll();
>+    });
>+  }
>+
>+  //need to allow for arbitrary network servers defined in PAC instead of a hardcoded moz-proxy.
>+  var ios = SpecialPowers.wrap(Components)
>+    .classes["@mozilla.org/network/io-service;1"]
>+    .getService(Components.interfaces.nsIIOService);
>+
>+  var pps = SpecialPowers.wrap(Components)
>+    .classes["@mozilla.org/network/protocol-proxy-service;1"]
>+    .getService();
>+
>+  var uri = ios.newURI("http://test", null, null);
>+  var pi = pps.resolve(uri, 0);
>+  mozproxy = "moz-proxy://" + pi.host + ":" + pi.port;
>+
>+  addLogin(mozproxy, "proxy_realm", "user", "pass");

I am baffled by this function.  :)

Are you adding an incorrect username/password (username 'user', password
'pass'), and then testing that we get a chance to provide a new password?

If so, could you add a comment to that effect?  If not, could you help me
understand what this code is for?

>+function finalizeFuncs() {
>+  for (var i = 0; i < finalizeFunctions.length; i++) {
>+    try {
>+      finalizeFunctions[i]();
>+    } catch (e) {
>+      // Throw the exception away.
>+    }

Are you getting exceptions here?  It's not clear to me why you don't care about
an exception, since it could mean that your login is not getting removed
successfully from the password manager.
Comment on attachment 653715 [details] [diff] [review]
Part 2: Implement asyncPromptAuth and promptAuth.

I don't think we should fall back to the original prompt service under any
circumstances because that will just create a new bug similar to this one --
you do something to cause us to fall back, and then the whole phone crashes.
Instead, we should just fail.

We're also inconsistent with our underscores here.  Either all of the internal
methods should have underscores, or none should.  Since the existing browser
element code uses the underscore, we should probably match that here.

This patch is very good.  We may need just one more review after this one.

>     if (args.promptType == 'prompt' ||
>-        args.promptType == 'confirm') {
>+        args.promptType == 'confirm' ||
>+        args.promptType == 'usernameandpassword' ||
>+        args.promptType == 'password') {

Can you add a test for a password prompt?  The test in the bug only tests
usernameandpassword prompts.

>diff --git a/dom/browser-element/BrowserElementParent.js b/dom/browser-element/BrowserElementParent.js
>@@ -239,16 +240,19 @@ function BrowserElementParent(frameLoade
>   defineDOMRequestMethod('getCanGoForward', 'get-can-go-forward');
> 
>   // Listen to mozvisibilitychange on the iframe's owner window, and forward it
>   // down to the child.
>   this._window.addEventListener('mozvisibilitychange',
>                                 this._ownerVisibilityChange.bind(this),
>                                 /* useCapture = */ false,
>                                 /* wantsUntrusted = */ false);
>+
>+  // Insert self into prompt service.

s/self/ourself/
s/prompt service/the prompt service/

>@@ -265,16 +269,61 @@ BrowserElementParent.prototype = {
>     return this._frameElement.ownerDocument.defaultView;
>   },
> 
>   get _windowUtils() {
>     return this._window.QueryInterface(Ci.nsIInterfaceRequestor)
>                        .getInterface(Ci.nsIDOMWindowUtils);
>   },
> 
>+  promptAuth: function(aHostname, aRealm, aReason, aType, aCallback) {
>+    let detail = {
>+      promptType:  aType,
>+      reason:      aReason,
>+      host:        aHostname,
>+      realm:       aRealm,
>+      returnValue: {
>+        ok: false,
>+        username: "",
>+        password: ""
>+      }
>+    };
>+
>+    let evt = this._createEvent('showmodalprompt', detail,
>+                                /* cancelable = */ true);
>+    debug("Event will have detail: " + JSON.stringify(evt.detail));
>+
>+    let self = this;
>+    let callbackCalled = false;
>+
>+    // Callback function.
>+    function doneCallback() {
>+      debug("Callback is called");

Nit: Maybe make this a little more specific (or otherwise just delete it).

>+      if (callbackCalled) {
>+        return;
>+      }
>+      callbackCalled = true;
>+      aCallback(evt.detail.returnValue.ok,
>+                evt.detail.returnValue.username,
>+                evt.detail.returnValue.password);
>+    }
>+
>+    // The name is unblock(), but it doesn't really block current
>+    // thread.
>+    defineAndExpose(evt.detail, 'unblock', function() {
>+      doneCallback();
>+    });
>+
>+    this._frameElement.dispatchEvent(evt);
>+
>+    if (!evt.defaultPrevented) {
>+      doneCallback();
>+    }
>+  },
>+
>   _sendAsyncMsg: function(msg, data) {
>     this._frameElement.QueryInterface(Ci.nsIFrameLoaderOwner)
>                       .frameLoader
>                       .messageManager
>                       .sendAsyncMessage('browser-element-api:' + msg, data);
>   },
> 
>   _recvHello: function(data) {
>diff --git a/dom/browser-element/BrowserElementPromptService.jsm b/dom/browser-element/BrowserElementPromptService.jsm
>--- a/dom/browser-element/BrowserElementPromptService.jsm
>+++ b/dom/browser-element/BrowserElementPromptService.jsm
>@@ -76,55 +79,403 @@ BrowserElementPrompt.prototype = {
> 
>   promptPassword: function(title, text, password, checkMsg, checkState) {
>     throw Cr.NS_ERROR_NOT_IMPLEMENTED;
>   },
> 
>   select: function(title, text, aCount, aSelectList, aOutSelection) {
>     throw Cr.NS_ERROR_NOT_IMPLEMENTED;
>   },
>+
>+  // Implement nsIAuthPrompt2 --------------------
>+
>+  promptAuth: function promptAuth(aChannel, aLevel, aAuthInfo) {

Is this covered by your testcase?

>+    debug("promptAuth");
>+    // Synchronous promptAuth() blocks main process.
>+    let [hostname, realm] = this.getAuthTarget(aChannel, aAuthInfo);
>+    let username = { value: "" };
>+    let password = { value: "" };
>+
>+    let ok = false;
>+    if (aAuthInfo.flags & Ci.nsIAuthInformation.ONLY_PASSWORD) {
>+      ok = this._syncPromptPassword(
>+        password, hostname, realm, this.getReason(aChannel, aAuthInfo));
>+    } else {
>+      ok = this._syncPromptUsernameAndPassword(
>+        username, password, hostname, realm, this.getReason(aChannel, aAuthInfo));
>+    }
>+
>+    this.setAuthInfo(aAuthInfo, username.value, password.value);
>+    return ok;
>+  },
>+
>+  asyncPromptAuth: function asyncPromptAuth(aChannel, aCallback, aContext, aLevel, aAuthInfo) {

We use |foo| rather than |aFoo| for arguments in this file, please change that
here and elsewhere.

>+    try {
>+      this.undef();
>+    } catch (e) {
>+      debug("Stack: " + e.stack.replace(/(?:\n@:0)?\s+$/m, '').replace(/^\(/gm, '{anonymous}(').split('\n'));
>+    }

Remove this?  Also, nice trick.  :)

>+    debug("asyncPromptAuth");
>+    let cancelable = null;
>+    try {

Is the try/catch there just to report the error to the console?

Gecko /ought to/ do that for you automatically, but I know it fails to do so in
a number of critical cases.

If you want to leave the try/catch, can we do

  asyncPromptAuth: function(...) {
    try {
      return this._asyncPromptAuthImpl(...);
    }
    catch(e) {
      ...
    }
  }

so as not to confuse things with a giant try/catch?

>+      let frame = this.getFrame(aChannel);
>+      if (!frame) {
>+        debug("Cannot get frame, asyncPromptAuth fail");
>+        throw Cr.NS_ERROR_FAILURE;
>+      }
>+
>+      let browserElementParent = BrowserElementPromptService.getBrowserElementParentForFrame(frame);

Nit: Please wrap this line (after the equals sign).

>+
>+      if (!browserElementParent) {
>+        debug("Fail to load browser element parent.");

Nit: s/Fail/Failed/

>+        throw Cr.NS_ERROR_FAILURE;
>+      }
>+
>+      cancelable = {
>+        QueryInterface: XPCOMUtils.generateQI([Ci.nsICancelable]),
>+        callback: aCallback,
>+        context: aContext,
>+        cancel: function() {
>+          this.callback.onAuthCancelled(this.context, false);
>+          this.callback = null;
>+          this.context = null;
>+        }
>+      };

JS doesn't require that all functions end with a return statement.  So even in
this function's current form (without moving this code into
_asyncPromptAuthImpl), you should do |let cancelable| here, and move the return
statement up into the try/catch.

Also, "cancelable" is a weird variable name.  Maybe "authConsumer",
"promptConsumer", or "consumer".

>+      let [hostname, httpRealm] = this.getAuthTarget(aChannel, aAuthInfo);
>+      let hashKey = aLevel + "|" + hostname + "|" + httpRealm;
>+      let asyncPrompt = this._asyncPrompts[hashKey];
>+      if (asyncPrompt) {
>+        asyncPrompt.consumers.push(cancelable);
>+        return cancelable;
>+      }
>+
>+      asyncPrompt = {
>+        consumers: [cancelable],
>+        channel: aChannel,
>+        authInfo: aAuthInfo,
>+        level: aLevel,
>+        inProgress : false,

Nit: |inProgress: false|.

>+        parent: browserElementParent

Please call this browserElementParent; "parent" can mean a lot of different
things, but in this case, the BEP is not the "parent" of the prompt -- if
anything, it's the "owner".

>+      };
>+
>+      this._asyncPrompts[hashKey] = asyncPrompt;
>+      this._doAsyncPrompt();
>+    } catch (e) {
>+      Cu.reportError("BrowserElementPromptService: " + e + "\n");
>+      throw e;
>+    }
>+    return cancelable;
>+  },
>+
>+  // Utilities for nsIAuthPrompt2 ----------------
>+
>+  _asyncPrompts: {},
>+  _asyncPromptInProgress: new WeakMap(),
>+  _doAsyncPrompt: function() {
>+    // Find the first prompt key, whose parent is not in async prompt progress.

Nit: No comma.

We use a comma in cases like this when the part after the comma is not
essential.  For example "my employer, which makes Firefox".  (I only have one
employer, so "which makes Firefox" isn't necessary for you to figure out which
one I'm talking about.)

But here, "whose parent is not in async prompt progress" is essential to
understanding what you're talking about -- you don't want the first prompt key,
but the first prompt key which meets a certian criterion.  Therefore, no comma.

Also, "whose parent is not in async prompt progress" doesn't match up with the
name of the variable (asyncPromptInProgress).

Maybe "Find a pending async prompt whose browser element parent does not
currently have a prompt in progress."

(Let me know if you'd prefer that I skip the pedantry and simply correct your
sentences; that's totally fine with me.)

>+    let hashKey = null;
>+    for (let key in this._asyncPrompts) {
>+      let prompt = this._asyncPrompts[key];
>+      if (!this._asyncPromptInProgress.get(prompt.parent)) {
>+        hashKey = key;
>+        break;
>+      }
>+    }

Also, this isn't going to process the pending prompts in FIFO order; the order
will be random (this is hashtable iteration).  That seems wrong.  (If you fix
this, then you can again say "Find /the first/ pending async prompt".  :)

>+    // Didn't find any available prompt, so just return.

Nit: "an available prompt", or "any available prompts".

>+    if (!hashKey)
>+      return;
>+
>+    let prompt = this._asyncPrompts[hashKey];
>+    let [hostname, httpRealm] = this.getAuthTarget(prompt.channel,
>+                                                   prompt.authInfo);
>+
>+    this._asyncPromptInProgress.set(prompt.parent, true);
>+    prompt.inProgress = true;
>+
>+    let self = this;
>+    let callback = function(ok, username, password) {
>+      debug("Callback is called, ok = " + ok + ", username = " + username);
>+
>+      // Here we got username and password provided by embedder, or
>+      // ok = false if the prompt is cancelled by embedder.

s/username/the username/

>+      delete self._asyncPrompts[hashKey];
>+      prompt.inProgress = false;
>+      self._asyncPromptInProgress.set(prompt.parent, false);

Might as well remove it instead, so that _asyncPromptInProgress doesn't grow
unnecessarily.

>+      self.setAuthInfo(prompt.authInfo, username, password);
>+      for each (let consumer in prompt.consumers) {
>+        debug("Consuming consumer");

Nit: Make this more descriptive ("Consuming prompt consumer"), or just remove it.

Also, we're not consuming the consumer.  We're...feeding the consumer, I guess,
although I've never heard anyone say that.  I might say "notifying prompt
consumer".

>+        if (!consumer.callback)
>+          // Not having a callback means that consumer didn't provide it
>+          // or canceled the notification
>+          continue;

Braces around this, please, since it's multi-line.

>+      // Process next prompt, if there is.
>+      self._doAsyncPrompt();

"Process the next prompt, if one is pending."

>+    let runnable = {
>+      run: function() {
>+        // Call promptAuth of browserElementParent, to show prompt.

Nit: "to show the prompt"

>+        prompt.parent.promptAuth(hostname, httpRealm, self.getReason(prompt.channel, prompt.authInfo),
>+                                 prompt.authInfo.flags & Ci.nsIAuthInformation.ONLY_PASSWORD ? 'password' : 'usernameandpassword',

Nit: Please split this long line (it might be cleanest to pull the ?: into a variable).

>+  getFrame: function(aChannel) {
>+    let loadContext = aChannel.notificationCallbacks.getInterface(Ci.nsILoadContext);

QueryInterface here?  Or does that not work?

If you need getInterface, please QI to nsIInterfaceRequestor first.  This might
work as-is, but AIUI it's relying on this object having been QI'ed to
nsIInterfaceRequestor sometime before now.  So we could break if that
code changed.

>+  getReason: function getReason(aChannel, aAuthInfo) {
>+    let scheme = aChannel.URI.scheme;
>+    if (scheme == 'http' ||
>+        scheme == 'https') {
>+      if (aAuthInfo.flags & Ci.nsIAuthInformation.AUTH_PROXY) {
>+        return 'httpproxy';
>+      } else {
>+        return 'httpauth';
>+      }
>+    } else if (scheme == 'ftp') {
>+      return 'ftplogin';
>+    }
>+  },

We should probably do something other than return undefined if we can't get the
reason.  Throwing an exception might work.

>+  setAuthInfo : function (aAuthInfo, aUsername, aPassword) {
>+    var flags = aAuthInfo.flags;
>+    if (flags & Ci.nsIAuthInformation.NEED_DOMAIN) {
>+      // Domain is separated from username by a backslash
>+      var idx = aUsername.indexOf("\\");
>+      if (idx == -1) {
>+        aAuthInfo.username = aUsername;
>+      } else {
>+        aAuthInfo.domain   = aUsername.substring(0, idx);
>+        aAuthInfo.username = aUsername.substring(idx+1);

Nit: |idx + 1|.

>+  _syncPromptUsernameAndPassword: function (
>+    username, password, host, realm, reason) {

Nit: Please break the line before "function".

>+    // expecting return value from embedder.
>+    // {
>+    //   string username,
>+    //   string password,
>+    //   bool   ok            // false for cancel.
>+    // };

"We expect the embedder to return an object of the form:"

Except we get this object BrowserElementChild, not from the embedder!

Also, please move this comment below the showModalPrompt call, since that's
where we care what the BEC returns.

>+    let rv = this._browserElementChild.showModalPrompt(
>+      this._win,
>+      {
>+        promptType:     "usernameandpassword",
>+        reason:         reason,
>+        host:           host,
>+        realm:          realm
>+      });
>+
>+    username.value = rv.username;
>+    password.value = rv.password;
>+    return rv.ok;
>+  },

>+  _syncPromptPassword: function (password, host, realm, reason) {
>+    // expecting return value from embedder.
>+    // {
>+    //   string password,
>+    //   bool   ok            // false for cancel.
>+    // };

Same here as above.

>+    let rv = this._browserElementChild.showModalPrompt(
>+      this._win,
>+      {
>+        promptType:     "password",
>+        reason:         reason,
>+        host:           host,
>+        realm:          realm
>+      });
>+
>+    password.value = rv.password;
>+    return rv.ok;
>+  }
>+};

>+function AuthPromptWrapper(oldImpl, browserElementImpl) {
>+  this._oldImpl = oldImpl;
>+  this._browserElementImpl = browserElementImpl;
>+}
>+
>+AuthPromptWrapper.prototype = {

If we're not falling back, you don't need AuthPromptWrapper, right?

> function BrowserElementPromptFactory(toWrap) {
>+  this._initialized = false;

Don't think you need this; see below.

>   this._wrapped = toWrap;
> }
> 
> BrowserElementPromptFactory.prototype = {
>   classID: Components.ID("{24f3d0cf-e417-4b85-9017-c9ecf8bb1299}"),
>   QueryInterface: XPCOMUtils.generateQI([Ci.nsIPromptFactory]),
> 
>   getPrompt: function(win, iid) {
>+    // It is possible for some object to get a prompt without passing
>+    // valid reference of window, like nsNSSComponent. In such case, we
>+    // should just fall back.
>+    if (!win)
>+      return this._wrapped.getPrompt(win, iid);

Remove the fallback here and elsewhere.

>     let browserElementChild =
>       BrowserElementPromptService.getBrowserElementChildForWindow(win);
>     if (!browserElementChild) {
>-      debug("Falling back to wrapped prompt service because " +
>-            "we can't find a browserElementChild for " +
>-            win + ", " + win.location);
>-      return this._wrapped.getPrompt(win, iid);
>+      if (iid.number === Ci.nsIAuthPrompt2.number) {
>+        // Because nsIAuthPrompt2 is called in parent process. If caller
>+        // wants nsIAuthPrompt2 and we cannot get BrowserElementchild,
>+        // it doesn't mean that we should fallback. It is possible we can
>+        // get the BrowserElementParent from nsIChannel that passed to
>+        // functions of nsIAuthPrompt2.
>+        debug("Use AuthPromptWrapper for nsIAuthPrompt2.");
>+        return new AuthPromptWrapper(
>+          this._wrapped.getPrompt(win, iid),
>+          new BrowserElementPrompt(win, undefined).QueryInterface(iid))
>+          .QueryInterface(iid);

Nit: Please indent inside the new AuthPromptWrapper parentheses.

>   _init: function() {
>+    if (this._initialized)
>+      return;

Are you seeing multiple calls to _init()?  That would be very strange.
I don't think you should need this.

>@@ -167,16 +519,27 @@ let BrowserElementPromptService = {
>+  mapFrameToBrowserElementParent: function(frame, browserElementParent) {
>+    debug("Adding a entry from frame to browserElementParent");
>+    if (this._browserElementParentMap)
>+      this._browserElementParentMap.set(frame, browserElementParent);

You shouldn't need this null check; nobody should be able to access
BrowserElementPromptService until after it has been import()'ed for the first
time.  And the first time it's import()'ed, we run the file, which calls
_init() (see the last line).  Calls to import() after the first call shouldn't
run the file.

(Of course we /will/ re-run the file if you call import() for the first time in
a new process, but that BrowserElementPromptService is completely different
from a BrowserElementPromptService in a different process.  But maybe that
explains why you observed multiple calls to init().)

>+  getBrowserElementParentForFrame: function(frame) {
>+    if (this._browserElementParentMap)
>+      return this._browserElementParentMap.get(frame);

No need for this null check, either.
Attachment #653715 - Flags: review?(justin.lebar+bug) → review-
(In reply to Justin Lebar [:jlebar] from comment #68)
> Comment on attachment 653715 [details] [diff] [review]
> Part 2: Implement asyncPromptAuth and promptAuth.
> 
> I don't think we should fall back to the original prompt service under any
> circumstances because that will just create a new bug similar to this one --
> you do something to cause us to fall back, and then the whole phone crashes.
> Instead, we should just fail.

Fall back part is added into this patch because this patch may
interfere with the desktop version Firefox. In desktop Firefox, when the
browser (not mozbrowser) navigates to a site that requires http
authentication, it shows a native style prompt before this patch is
applied. This patch would prevent the native prompt from being displayed,
because no BrowserElementChild is created in such case. If we just throw
an exception in getPrompt(), browser will not show the prompt, and users
won't have a chance to enter their username/password. This also makes
mochitest of some other modules become failure if it doesn't fall back.
(In reply to Justin Lebar [:jlebar] from comment #65)
> Can you push this to try?  Let me know if you need me to.  I'd also be happy
> to vouch for L1 access, if you don't have it.

Yeah, I've pushed this test to try. Here's the result https://tbpl.mozilla.org/?tree=Try&rev=b6eb5348e8ce.
>>+  getFrame: function(aChannel) {
>>+    let loadContext = aChannel.notificationCallbacks.getInterface(Ci.nsILoadContext);
>
>QueryInterface here?  Or does that not work?

You should continue to use getInterface.
> Fall back part is added into this patch because this patch may
> interfere with the desktop version Firefox. 

Oh, right!  We override the prompt service, so we need to fall back for prompts outside <mozbrowser>.

I still don't think we should fall back for prompts coming from within mozbrowser, though.  And (in a separate bug, which I'll file) we should add a setting somewhere which disables the fallback altogether on B2G, since it can't do any good.
promptAuth() is used in FTP connection or when asyncPromptAuth() fail, and ONLY_PASSWORD is set only for FTP, but Mochitest seems not providing an FTP server. Is there a way that we can test these piece of code?
(In reply to Patrick Wang [:kk1fff] from comment #73)
> promptAuth() is used in FTP connection or when asyncPromptAuth() fail, and
> ONLY_PASSWORD is set only for FTP, but Mochitest seems not providing an FTP
> server. Is there a way that we can test these piece of code?

I asked on IRC, and it doesn't look like we can we write mochitests which use FTP, without writing our own FTP server.  We should write a manual testcase for QA to run, but we can do that afterwards.
Can you help me understand what's going on in

>+function testProxyStart() {

in the testcase?
(In reply to Justin Lebar [:jlebar] from comment #75)
> Can you help me understand what's going on in
> 
> >+function testProxyStart() {
> 
> in the testcase?

This function is wrote with a reference http://hg.mozilla.org/mozilla-central/file/e509c7472f30/toolkit/components/passwordmgr/test/test_prompt_async.html#l72. I though that we may need to register a proxy to make gecko handle the http 407 response. But I run the test code again without addLogin, it still works, so it may be not necessary.

By the way, I think that we could modify the logic slightly. Instead of having two types of dialog ('usernameandpassword' or 'password'), we could just tell embedder that the prompt type is 'usernameandpassword' with an additional field 'isOnlyPassword'. And 'reason' field can also be replaced by two additional fields: 'scheme' and 'isProxy'.

The data structure that we passed to embedder will look like:
{
  promptType:     "usernameandpassword",
  scheme:         channel.URI.scheme,
  isProxy:        authInfo.flags & Ci.nsIAuthInformation.AUTH_PROXY,
  isOnlyPassword: authInfo.flags & Ci.nsIAuthInformation.ONLY_PASSWORD,
  host:           host,
  realm:          realm
}

So we just bypass the information that necko provides. And data field can be verified with xpcshell. How do you think?
Summarize of update of this patch:
1. Fix previous version with review comments.
2. Read property "browser.prompt.allowNative" (interim property name) to decide if getPrompt fall back when any failure occurs.
3. Update form of object that passed to embedder to:
{
  promptType:     "usernameandpassword",
  scheme:         // URI scheme, http, ftp...etc.
  isProxy:        // true if the auth is for proxy
  isOnlyPassword: // true if we only ask user to provide password.
  host:           // Host part of uri.
  realm:          // Authentication realm.
}
4. Update promptAuth.
Attachment #653715 - Attachment is obsolete: true
Attachment #655557 - Flags: review?(justin.lebar+bug)
Attached patch Test case (obsolete) — — Splinter Review 
Summarize this patch
1. Update previous version with comment.
2. Add xpcshell test to test if BrowserElementPrompt generates detail object from nsAuthInoformation and nsIChannel.
Attachment #653716 - Attachment is obsolete: true
Attachment #653716 - Flags: review?(justin.lebar+bug)
Attachment #655558 - Flags: review?(justin.lebar+bug)
>3. Update form of object that passed to embedder to:
>{
>  promptType:     "usernameandpassword",
>  scheme:         // URI scheme, http, ftp...etc.
>  isProxy:        // true if the auth is for proxy
>  isOnlyPassword: // true if we only ask user to provide password.
>  host:           // Host part of uri.
>  realm:          // Authentication realm.
>}

Some comments on this:

1. We shouldn't have promptType == "usernameandpassword" if we're asking only
for a password and not a username; that's misleading.  So I'd prefer the old
way, where we had "usernameandpassword" and "password" prompts.

Alternatively, you could have

  {
    needsUsername: boolean
    needsPassword: boolean
  }

because it's conceivable that we might want to prompt for only a username
sometimes.  But I think that's over-engineering.

2. Given that this object is almost completely different form the
showmodalprompt object (which is {promptType, title, message, returnValue}),
I'm not sure it makes sense to send auth requests as a "showmodalprompt" event
anymore.

What would you think if we fired these as "authenticationrequest" events
instead?  Then we could also change the e.detail.unblock() to
e.detail.authenticate(username, password) and e.detail.cancel(), which would be
cleaner than the way we do it now.

3. "And 'reason' field can also be replaced by two additional fields: 'scheme'
and 'isProxy'."

What is the host/scheme for proxy requests?  It's good to avoid fields which
are meaningful only sometimes.  So maybe we should have a separate event for
proxy auth, like so:

networkauthentication event

  {
    needsUsername: true/false,
    needsPassword: true/false,
    scheme: "http" or "ftp",
    host:
    realm:
  }

At least one of needsUsername or needsPassword must be true.

proxyauthentication event

  {
    needsUsername: true/false,
    needsPassword: true/false,
    realm: (Is this meanigful?)
    ???
  }
Mostly nits here.  The main thing we need to figure out is the interface
exposed to the embedder.

>+  promptAuth: function promptAuth(channel, level, authInfo) {
>+    debug("promptAuth");
>+    // Synchronous promptAuth() blocks main process.
>+    let [hostname, realm] = this._getAuthTarget(channel, authInfo);
>+    let username = { value: "" };
>+    let password = { value: "" };
>+
>+    let frame = this._getFrame(channel);
>+    if (!frame) {
>+      debug("Cannot get frame, asyncPromptAuth fail");
>+      throw Cr.NS_ERROR_FAILURE;
>+    }
>+
>+    let browserElementParent =
>+      BrowserElementPromptService.getBrowserElementParentForFrame(frame);
>+
>+    if (!browserElementParent) {
>+      debug("Failed to load browser element parent.");
>+      throw Cr.NS_ERROR_FAILURE;
>+    }
>+
>+    let ok = false;
>+    ok = this._syncPromptUsernameAndPassword(
>+      username, password, this._formAuthDetail(channel, authInfo),
>+      browserElementParent);

_syncPromptUsernameAndPassword is also called when we want only a password,
right?  If so, let's call it _doSyncPrompt to matcy _doAsyncPrompt. Or maybe
better: inline the function in here, since it's not called from anywhere else.

>+  asyncPromptAuth: function asyncPromptAuth(channel, callback, context, level, authInfo) {
>+    debug("asyncPromptAuth");
>+    let frame = this._getFrame(channel);
>+    if (!frame) {
>+      debug("Cannot get frame, asyncPromptAuth fail");
>+      throw Cr.NS_ERROR_FAILURE;
>+    }
>+
>+    let browserElementParent =
>+      BrowserElementPromptService.getBrowserElementParentForFrame(frame);
>+
>+    if (!browserElementParent) {
>+      debug("Failed to load browser element parent.");
>+      throw Cr.NS_ERROR_FAILURE;
>+    }
>+
>+    let consumer = {
>+      QueryInterface: XPCOMUtils.generateQI([Ci.nsICancelable]),
>+      callback: callback,
>+      context: context,
>+      cancel: function() {
>+        this.callback.onAuthCancelled(this.context, false);
>+        this.callback = null;
>+        this.context = null;
>+      }
>+    };
>+
>+    let [hostname, httpRealm] = this._getAuthTarget(channel, authInfo);
>+    let hashKey = level + "|" + hostname + "|" + httpRealm;
>+    let asyncPrompt = this._asyncPrompts[hashKey];
>+    if (asyncPrompt) {
>+      asyncPrompt.consumers.push(consumer);
>+      return consumer;
>+    }
>+
>+    asyncPrompt = {
>+      consumers: [consumer],
>+      channel: channel,
>+      authInfo: authInfo,
>+      level: level,
>+      inProgress: false,
>+      browserElementParent: browserElementParent
>+    };
>+
>+    this._asyncPrompts[hashKey] = asyncPrompt;
>+    this._doAsyncPrompt();
>+    return consumer;
>+  },
>+
>+  // Utilities for nsIAuthPrompt2 ----------------
>+
>+  _asyncPrompts: {},
>+  _asyncPromptInProgress: new WeakMap(),
>+  _doAsyncPrompt: function() {
>+    // Find key of a prompt whose browser element parent is not in async prompt
>+    // progress.

"Find /the/ key of a prompt whose browser element parent /does not have/ an
async prompt /in/ progress."

>+    let hashKey = null;
>+    for (let key in this._asyncPrompts) {
>+      let prompt = this._asyncPrompts[key];
>+      if (!this._asyncPromptInProgress.get(prompt.browserElementParent)) {
>+        hashKey = key;
>+        break;
>+      }
>+    }
>+
>+    let prompt = this._asyncPrompts[hashKey];
>+    let [hostname, httpRealm] = this._getAuthTarget(prompt.channel,
>+                                                   prompt.authInfo);

Nit: Extra space before "prompt.authInfo" to line it up.

>+    this._asyncPromptInProgress.set(prompt.browserElementParent, true);
>+    prompt.inProgress = true;
>+
>+    let self = this;
>+    let callback = function(ok, username, password) {
>+      debug("Callback is called, ok = " + ok + ", username = " + username);

Nit: "Async auth callback is called", or something.

>+      // Here we got the username and password provided by embedder, or
>+      // ok = false if the prompt is cancelled by embedder.

s/is/was/ (because you used the past tense "got" rather than the present tense
"get").

>+        try {
>+          if (ok) {
>+            debug("Ok, call onAuthAvailable to finish auth");

Nit: s/call/calling

("Call" here is the imperative mood: You're asking me to do something.  But
this log message describes what you're doing, so you shouldn't use the
imperative.)

>+            consumer.callback.onAuthAvailable(consumer.context, prompt.authInfo);
>+          } else {
>+            debug("Cancelled, call onAuthCancelled to finish auth.");

s/call/calling

>+            consumer.callback.onAuthCancelled(consumer.context, true);
>+          }
>+        } catch (e) { /* Throw away exceptions caused by callback */ }
>+      }
>+
>+      // Process next prompt, if one is pending.

Nit: Process /the/ next prompt

>+  _getFrame: function(channel) {

Nit: _getFrameFromChannel.

>+  _formAuthDetail: function(channel, authInfo) {

Nit: _createAuthDetail would be more idiomatic.  ("formAuthDetail" can be read
as "auth detail for a form", which doesn't make much sense.  Perhaps because of
the ambiguity, we don't usually use "form" as a verb in code.)

>+  _syncPromptUsernameAndPassword: function (username, password, detail, bep) {
>+    let blocked = true;
>+    let ok = false;
>+    let thread = Services.tm.currentThread;
>+    let callback = function(cb_ok, cb_username, cb_password) {
>+      username.value = cb_username;
>+      password.value = cb_password;
>+      ok = cb_ok;
>+      thread.dispatch(
>+        {
>+          run: function() {
>+            blocked = false;
>+          }
>+        },
>+        Ci.nsIThread.DISPATCH_NORMAL);
>+    };
>+
>+    bep.promptAuth(detail, callback);
>+
>+    // After sending an event to embedder, we block ourself and wait
>+    // for return.

"After sending /the/ event to /the/ embedder, we block ourself and wait for
/the embedder to unblock us/."

It's not clear to me why you have to set |blocked = false| from a runnable, but
maybe you have a good reason for that.  Not a big deal either way.

> function BrowserElementPromptFactory(toWrap) {
>   this._wrapped = toWrap;
> }
> 
> BrowserElementPromptFactory.prototype = {
>   classID: Components.ID("{24f3d0cf-e417-4b85-9017-c9ecf8bb1299}"),
>   QueryInterface: XPCOMUtils.generateQI([Ci.nsIPromptFactory]),
> 
>+  _shouldFallback: function() {

"Fallback" is a noun, "fall back" is a verb.  So here, it should be
"_shouldFallBack".

But actually, I think _mayUseNativePrompt would be a better name.

(This rule is dying -- for example, lots of sites ask you "to login", rather
than "to log in" -- but it's not dead yet!)

>+    try {
>+      return Services.prefs.getBoolPref("browser.prompt.allowNative");
>+    } catch (e) {
>+      // If fall back is not allowed, it will be set explicitly.
>+      return true;
>+    }
>+  },
>+
>+  _fallbackIfAllowed: function(win, iid, err) {

Perhaps _getNativePromptIfAllowed would be clearer.

>   getPrompt: function(win, iid) {
>+    // It is possible for some object to get a prompt without passing
>+    // valid reference of window, like nsNSSComponent. In such case, we
>+    // should just fall back.

fall back /to the native prompt service/.

>     // Try to find a browserelementchild for the window.
>-    if (iid.number != Ci.nsIPrompt.number) {
>-      debug("Falling back to wrapped prompt service because " +
>-            "we don't recognize the requested IID (" + iid + ", " +
>-            "nsIPrompt=" + Ci.nsIPrompt);
>-      return this._wrapped.getPrompt(win, iid);
>+    if (iid.number != Ci.nsIPrompt.number &&
>+        iid.number != Ci.nsIAuthPrompt.number &&
>+        iid.number != Ci.nsIAuthPrompt2.number) {
>+      debug("We don't recognize the requested IID (" + iid + ", " +
>+            "allowed IID: " +
>+            "nsIPrompt=" + Ci.nsIPrompt + ", " +
>+            "nsIAuthPrompt=" + Ci.nsIAuthPrompt + ", " +
>+            "nsIAuthPrompt2=" + Ci.nsIAuthPrompt2 + ")");
>+      return this._fallbackIfAllowed(win, iid, Cr.NS_ERROR_INVALID_ARG);
>     }

Move the comment about finding a browserelementchild down here.

>     let browserElementChild =
>       BrowserElementPromptService.getBrowserElementChildForWindow(win);
>     if (!browserElementChild) {
>-      debug("Falling back to wrapped prompt service because " +
>-            "we can't find a browserElementChild for " +
>-            win + ", " + win.location);
>-      return this._wrapped.getPrompt(win, iid);
>+      if (iid.number === Ci.nsIAuthPrompt2.number) {
>+        debug("Attmpting to get an nsIAuthPrompt2 without an available " +
>+              "BrowserElementChild.");
>+        // Because nsIAuthPrompt2 is called in parent process. If caller
>+        // wants nsIAuthPrompt2 and we cannot get BrowserElementchild,
>+        // it doesn't mean that we should fallback. It is possible that we can
>+        // get the BrowserElementParent from nsIChannel that passed to
>+        // functions of nsIAuthPrompt2.
>+        if (this._shouldFallback()) {
>+          return new AuthPromptWrapper(
>+              this._wrapped.getPrompt(win, iid),
>+              new BrowserElementPrompt(win, undefined).QueryInterface(iid))
>+            .QueryInterface(iid);
>+        } else {
>+          // Falling back is not allowed, so we don't need wrap the
>+          // BrowserElementPrompt.
>+          new BrowserElementPrompt(win, undefined).QueryInterface(iid);

Don't you need to return this?

>+        }
>+      } else {
>+        debug("We can't find a browserEleamentChild for " +
>+              win + ", " + win.location);
>+        return this._fallbackIfAllowed(win, iid, Cr.NS_ERROR_FAILURE);
>+      }
>     }
> 
>     debug("Returning wrapped getPrompt for " + win);
>     return new BrowserElementPrompt(win, browserElementChild)
>                                    .QueryInterface(iid);

What would you think of making a separate BrowserElementAuthPrompt object,
instead of shoehorning it in like we do here?  It's pretty ugly that when we
make an auth prompt, we have a null browserelementchild, whereas otherwise, the
BEC is never null.  And the BrowserElementPrompt object returned in the child
process (the retun statement just above) does not correctly implement
nsIAuthPrompt2, even though it will happily QI to nsIAuthPrompt2.

>+  // For xpcshell test.
>+  _getBrowserElementPrompt: function() {
>+    return new BrowserElementPrompt(null, null);
>+  }
> };

This is kind of ugly, but I'll wait to pass judgement until I read the test.
Attachment #655557 - Flags: review?(justin.lebar+bug) → feedback+
Comment on attachment 655558 [details] [diff] [review]
Test case

If possible, could you put the xpcshell test inside a directory called "xpcshelltests", or "unittest"?  I say "if possible" because I'm not sure if xpcshell needs to be in a directory called "test" or something silly like that.

Anyway, this looks good, but please change _getBrowserElementPrompt() to _testOnlygetBrowserElementPrompt() or something, so nobody will accidentally use this function and expect it to do something good.
Attachment #655558 - Flags: review?(justin.lebar+bug) → review+
Grep tells me the most common name for a directory containing xpcshell tests is 'unit'.  But it doesn't need to have that name, apparently.
(In reply to Justin Lebar [:jlebar] from comment #80)
> What would you think of making a separate BrowserElementAuthPrompt object,
> instead of shoehorning it in like we do here?  It's pretty ugly that when we
> make an auth prompt, we have a null browserelementchild, whereas otherwise,
> the
> BEC is never null.  And the BrowserElementPrompt object returned in the child
> process (the retun statement just above) does not correctly implement
> nsIAuthPrompt2, even though it will happily QI to nsIAuthPrompt2.
> 
It sounds better than current implementation, I'll do this in next update.
(In reply to Justin Lebar [:jlebar] from comment #79)
> 1. We shouldn't have promptType == "usernameandpassword" if we're asking only
> for a password and not a username; that's misleading.  So I'd prefer the old
> way, where we had "usernameandpassword" and "password" prompts.
> 
> Alternatively, you could have
> 
>   {
>     needsUsername: boolean
>     needsPassword: boolean
>   }
> 
> because it's conceivable that we might want to prompt for only a username
> sometimes.  But I think that's over-engineering.
> 
> 2. Given that this object is almost completely different form the
> showmodalprompt object (which is {promptType, title, message, returnValue}),
> I'm not sure it makes sense to send auth requests as a "showmodalprompt"
> event
> anymore.
> 
> What would you think if we fired these as "authenticationrequest" events
> instead?  Then we could also change the e.detail.unblock() to
> e.detail.authenticate(username, password) and e.detail.cancel(), which would
> be
> cleaner than the way we do it now.
> 
> 3. "And 'reason' field can also be replaced by two additional fields:
> 'scheme'
> and 'isProxy'."
> 
> What is the host/scheme for proxy requests?  It's good to avoid fields which
> are meaningful only sometimes.  So maybe we should have a separate event for
> proxy auth, like so:
> 
> networkauthentication event
> 
>   {
>     needsUsername: true/false,
>     needsPassword: true/false,
>     scheme: "http" or "ftp",
>     host:
>     realm:
>   }
> 
> At least one of needsUsername or needsPassword must be true.
> 
> proxyauthentication event
> 
>   {
>     needsUsername: true/false,
>     needsPassword: true/false,
>     realm: (Is this meanigful?)
>     ???
>   }

Proxy authentication has a realm in its Proxy-Authenticate response header. Host
in proxy auth may also available to indicate the the proxy server. Scheme may not
available in proxy auth (on comment in Android version of asyncPomptAuth
implementation[1]).

It looks that the original way (usernameandpassword/password) is more
reasonable. Also, the embedder can extract scheme from host string, host
string is in URI form. By examining scheme, embedder can also tell if the
auth is for a proxy, because when the auth is for proxy, the scheme will
be "moz-proxy". So I think we may only need to send realm and host to
embedder.

I would like to propose following event names and detail object that
send with event.

event: "authenticationrequired"
  {
    realm:
    host:
    function authenticate(username, password)
    function cancel()
  }

event: "authenticationonlypassword"
  {
    username: // asyncPromptAuth's caller would provide the username it is about to use.
    realm:
    host:
    function authenticate(password)
    function cancel()
  }

It's much like original way, except the event is new.

[1] http://hg.mozilla.org/mozilla-central/file/0eebcc79ee05/mobile/android/components/PromptService.js#l683
Jonas just pointed out that proxy auth affects all connections on your device, not just the connections in the browser (right?).  If so, we shouldn't let the browser app authenticate you to your proxy; that should go through some trusted system service.

So for now, maybe we should just disable this proxy stuff and fail on proxy auth.

> event: "authenticationonlypassword"

Do we always have a username when !!(authInfo.flags & Ci.nsIAuthInformation.ONLY_PASSWORD) is true?

How about events

  "usernameandpasswordrequired"
  "passwordrequired"

?  We should have parallelism between the two event names.
(In reply to Justin Lebar [:jlebar] from comment #85)
> Jonas just pointed out that proxy auth affects all connections on your
> device, not just the connections in the browser (right?).  If so, we
> shouldn't let the browser app authenticate you to your proxy; that should go
> through some trusted system service.
> 
> So for now, maybe we should just disable this proxy stuff and fail on proxy
> auth.
> 
Ok

> 
> Do we always have a username when !!(authInfo.flags &
> Ci.nsIAuthInformation.ONLY_PASSWORD) is true?
> 
> How about events
> 
>   "usernameandpasswordrequired"
>   "passwordrequired"
> 
> ?  We should have parallelism between the two event names.
Yeah. And in Android and desktop versions, browser shows the username in the prompt to tell user what username is going to be used for login. I think we may provide username as well.

The event names sound good.
> And in Android and desktop versions, browser shows the username in the prompt to tell 
> user what username is going to be used for login.

That username is coming from the network, not from some Firefox password manager?  If so, sounds good to me.
> proxy auth affects all connections on your device, not just the connections in
> the browser (right?).  If so, we shouldn't let the browser app authenticate
> you to your proxy; that should go through some trusted system service.

Ugh.  Yes, unless we "jarify" auth, it will be system-wide.   I don't personally think this is a v1-block-worthy problem (it's actually a feature in its own way: the fewer auth prompts the better, and it's unlikely a user want to allow app A to get into site foo but not app B).  As a practical attack vector it seems a little farfetched to me (is evil.app really going to wait around for some other app to provide it with basic auth credentials so it can read the low-value sites typically guarded by HTTP auth?).  But secteam might disagree.  

Do we have a sense of how much work it will be to build a trusted system service for this?
> Ugh.  Yes, unless we "jarify" auth, it will be system-wide.

My comment was specifically about proxy auth, but you're saying that HTTP auth will be system wide too?  That's unfortunate, but I agree it probably shouldn't block.
I'm even less worried about proxy auth--IMO it's not a security model to rely on apps with permission to access the Internet not connecting because you haven't entered proxy auth for them.
The question is more whether you want to trust your browser app (or any other random app which uses mozbrowser and happens to be the first one you touch when you're behind a proxy) with your proxy password.
(In reply to Justin Lebar [:jlebar] from comment #87)
> That username is coming from the network, not from some Firefox password
> manager?  If so, sounds good to me.

I asked on mailing list. The username is from ftp url. Like ftp://user@ftp.site/.

https://groups.google.com/forum/?fromgroups=#!topic/mozilla.dev.tech.network/8c1trzWKeNs
Summarize this update:
1. Let authenticate prompt for proxy fail.
2. Send event to embedder as description in comment 84 and comment 86.
3. Separate the implementation of nsIAuthPrompt2 from BrowserElementPrompt class.
Attachment #655557 - Attachment is obsolete: true
Attachment #656821 - Flags: review?(justin.lebar+bug)
Attached patch Test case (obsolete) — — Splinter Review 
Summarize the test case update:
1. Move xpcshell test case to unit/ directory.
2. Add a test to test the cancel() function, a callback function of 'usernameandpasswordrequired' event.
Attachment #655558 - Attachment is obsolete: true
Attachment #656825 - Flags: review?(justin.lebar+bug)
Attached patch Test case (obsolete) — — Splinter Review 
Remove SJS for http 407 response from Makefile.in
Attachment #656825 - Attachment is obsolete: true
Attachment #656825 - Flags: review?(justin.lebar+bug)
Attachment #656826 - Flags: review?(justin.lebar+bug)
Attachment #654130 - Attachment is obsolete: true
Comment on attachment 656843 [details] [diff] [review]
Part 1: Obtain frame from nsIChannel

This is the same as the old part 1, just rebased to tip (so I could push it to try).
Attachment #656843 - Flags: review+
Comment on attachment 656821 [details] [diff] [review]
Part 2: Implement asyncPromptAuth and promptAuth.

>diff --git a/dom/browser-element/BrowserElementChild.js b/dom/browser-element/BrowserElementChild.js
>--- a/dom/browser-element/BrowserElementChild.js
>+++ b/dom/browser-element/BrowserElementChild.js
>@@ -201,17 +201,18 @@ BrowserElementChild.prototype = {
> 
>     args.windowID = { outer: utils.outerWindowID,
>                       inner: this._tryGetInnerWindowID(win) };
>     sendAsyncMsg('showmodalprompt', args);
> 
>     let returnValue = this._waitForResult(win);
> 
>     if (args.promptType == 'prompt' ||
>-        args.promptType == 'confirm') {
>+        args.promptType == 'confirm' ||
>+        args.promptType == 'usernameandpassword') {

Presumably you need to update this to 'usernameandpasswordrequired' and
'passwordrequired'?  I guess we'll see if the test fails on try.  :)

>diff --git a/dom/browser-element/BrowserElementParent.js b/dom/browser-element/BrowserElementParent.js
>@@ -265,16 +269,69 @@ BrowserElementParent.prototype = {
>     return this._frameElement.ownerDocument.defaultView;
>   },
> 
>   get _windowUtils() {
>     return this._window.QueryInterface(Ci.nsIInterfaceRequestor)
>                        .getInterface(Ci.nsIDOMWindowUtils);
>   },
> 
>+  promptAuth: function(authDetail, callback) {
>+    let evt;
>+    let self = this;
>+    let callbackCalled = false;
>+    let cancelCallback = function() {
>+      if (!callbackCalled) {
>+        callbackCalled = true;
>+        callback(false, "", "");

Nit: Pass null instead of "" here, if that works.

Also, move cancelCallback closer to where it's used, please.

>diff --git a/dom/browser-element/BrowserElementPromptService.jsm b/dom/browser-element/BrowserElementPromptService.jsm
>     throw Cr.NS_ERROR_NOT_IMPLEMENTED;
>   },
> 
>   select: function(title, text, aCount, aSelectList, aOutSelection) {
>     throw Cr.NS_ERROR_NOT_IMPLEMENTED;
>   },
> };
> 
>+
>+function BrowserElementAuthPrompt() {
>+}
>+
>+BrowserElementAuthPrompt.prototype = {
>+  QueryInterface: XPCOMUtils.generateQI([Ci.nsIAuthPrompt2]),
>+
>+  promptAuth: function promptAuth(channel, level, authInfo) {
>+    debug("promptAuth");
>+
>+    // We don't authenticate proxy currently

Nit: s/proxy/to proxies

>+    if (authInfo.flags & Ci.nsIAuthInformation.AUTH_PROXY) {
>+      throw Cr.NS_ERROR_FAILURE;
>+    }
>+
>+    // Synchronous promptAuth() blocks main process.

I'm not sure why this comment is here, specifically.

>+    // After sending an event to embedder, we block ourself and wait
>+    // for return.
>+    debug("Start blocking");

Nit: Please put "promptAuth" in this debug message so we know who's blocking.

>+    while (blocked) {
>+      thread.processNextEvent(/* mayWait = */ true);
>+    }

Hmmm...this is a probem I should have caught earlier: If the page we're blocked
on gets destroyed (or the process containing that page crashes), we'll never
exit this nested event loop!

I don't see a simple way to get around this.  We just should not block the
parent on the child -- in other words, nobody should call promptAuth from the
parent if the prompt will be shown in the child.

We only use sync promptAuth for FTP auth, right?  I don't like taking a knife
to your hard work here, but maybe we should give up on sync promptAuth for this
bug and just return failure.

Sorry I didn't catch this earlier.  :(  This r- is entirely my fault.

>+    debug("Unblocked");

Nit: Please put "promptAuth" in this debug message.

>+  _mayUseNativePrompt: function() {
>+    try {
>+      return Services.prefs.getBoolPref("browser.prompt.allowNative");
>+    } catch (e) {
>+      // If fall back is not allowed, it will be set explicitly.

Sorry, this is kind of confusing.  How about "Default to true."
Attachment #656821 - Flags: review?(justin.lebar+bug) → review-
Comment on attachment 656826 [details] [diff] [review]
Test case

> Instead, we will be leaded to fail message

The past tense of "lead" is "led", not "leaded".  :)
Attachment #656826 - Flags: review?(justin.lebar+bug) → review+
Comment on attachment 656821 [details] [diff] [review]
Part 2: Implement asyncPromptAuth and promptAuth.

You should also post a patch which sets browser.prompt.allowNative to false for b2g.
(In reply to Justin Lebar [:jlebar] from comment #99)

> Presumably you need to update this to 'usernameandpasswordrequired' and
> 'passwordrequired'?  I guess we'll see if the test fails on try.  :)
> 
I should remove this line, we don't use this function in asyncPromptAuth anymore.

> >+    while (blocked) {
> >+      thread.processNextEvent(/* mayWait = */ true);
> >+    }
> 
> Hmmm...this is a probem I should have caught earlier: If the page we're
> blocked
> on gets destroyed (or the process containing that page crashes), we'll never
> exit this nested event loop!
> 
Oh... that's true. Because no one can set the flag to false in those situations.

> I don't see a simple way to get around this.  We just should not block the
> parent on the child -- in other words, nobody should call promptAuth from the
> parent if the prompt will be shown in the child.
> 
> We only use sync promptAuth for FTP auth, right?  I don't like taking a knife
> to your hard work here, but maybe we should give up on sync promptAuth for
> this
> bug and just return failure.
I agree with you, this part can provide very little improvement,
but it seems to cause more problems. I think removing this would be better.
As a general rule, we never block a parent process on one of its descendants.  This is one good reason for that.

I'm sorry again for not catching this sooner.
1. Remove implementation of promptAuth().
2. Add a property "browser.prompt.allowNative" in b2g/app/b2g.js.
3. Remove "args.promptType == 'usernameandpassword'" from BrowserElementChild.js

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=de65c745fbaf
Attachment #656821 - Attachment is obsolete: true
Attachment #657200 - Flags: review?(justin.lebar+bug)
Comment on attachment 657200 [details] [diff] [review]
Part 2: Implement asyncPromptAuth and promptAuth.

>diff --git a/dom/browser-element/BrowserElementParent.js b/dom/browser-element/BrowserElementParent.js
>+  promptAuth: function(authDetail, callback) {
>+    let evt;
>+    let self = this;
>+    let callbackCalled = false;
>+
>+    if (authDetail.isOnlyPassword) {

If this can be triggered by an HTTP request to http://jlebar@foo.com, then we should test it.

(Note that loading http://jlebar@foo.com on my desktop build triggers a prompt
even before the HTTP auth!  I dunno what will happen on B2G or in your tests.)

If OTOH we can't figure out how to test it, we should just fail.

But we've dragged this bug on long enough.  I think we can handle this test (and possible changes) in a follow-up.

>+  // For xpcshell test.
>+  _testOnlygetBrowserElementPrompt: function() {

Do we need the xpcshell test anymore, now that we're not supporting FTP?

r=me.  Let's remove the xpcshell test if you think we don't need it, check this in, and worry about the passwordonly case in a follow-up.
Attachment #657200 - Flags: review?(justin.lebar+bug) → review+
One nit:

>+  _testOnlygetBrowserElementPrompt: function() {

If we keep the xpcshell test, can we make this testOnlyGetBrowserElementPrompt (capitalize the "o")?
(In reply to Justin Lebar [:jlebar] from comment #106)
> Comment on attachment 657200 [details] [diff] [review]
> Part 2: Implement asyncPromptAuth and promptAuth.
> 
> 
> If this can be triggered by an HTTP request to http://jlebar@foo.com, then
> we should test it.
> 
> (Note that loading http://jlebar@foo.com on my desktop build triggers a
> prompt
> even before the HTTP auth!  I dunno what will happen on B2G or in your
> tests.)
> 
I tested with this form of url, but it doesn't trigger OnlyPassword.

In desktop firefox, a prompt shows to confirm the username. This prompt doesn't show in mozbrowser case in my mochitest and b2g.

> If OTOH we can't figure out how to test it, we should just fail.
> 
I think it's not easy to test it now. I'll remove the "onlyPassword" part and let it fail.

> But we've dragged this bug on long enough.  I think we can handle this test
> (and possible changes) in a follow-up.
> Do we need the xpcshell test anymore, now that we're not supporting FTP?
> 
> r=me.  Let's remove the xpcshell test if you think we don't need it, check
> this in, and worry about the passwordonly case in a follow-up.
If we don't handle the password-only prompt, maybe the mochitest is enough to cover all of the code we've done. Let me remove the xpcshell.

Thanks very much for reviewing my work and giving me instructions :) . Let me submit one more version with the modification we've talked above.
Summarize this patch:
1. Remove the part of password-only prompt.
2. Remove _testOnlygetBrowserElementPrompt(), which is for xpcshell.
Attachment #657200 - Attachment is obsolete: true
Attachment #657814 - Flags: review?(justin.lebar+bug)
Attached patch Test case — — Splinter Review 
Remove xpcshell test. Pushed to try server: https://tbpl.mozilla.org/?tree=Try&rev=d42f1eca739b
Attachment #656826 - Attachment is obsolete: true
Attachment #657825 - Flags: review?(justin.lebar+bug)
Comment on attachment 657814 [details] [diff] [review]
Part 2: Implement asyncPromptAuth and promptAuth.

This review is on the interdiff:

  interdiff  <(curl -L https://bug775464.bugzilla.mozilla.org/attachment.cgi?id=657200) <(curl -L https://bug775464.bugzilla.mozilla.org/attachment.cgi?id=657814)

diff -u b/dom/browser-element/BrowserElementParent.js b/dom/browser-element/BrowserElementParent.js
     if (authDetail.isOnlyPassword) {
-      let detail = {
-        username: authDetail.username,
-        host:     authDetail.host,
-        realm:    authDetail.realm
-      };
-
-      evt = this._createEvent('passwordrequired', detail,
-                              /* cancelable */ true);
-      defineAndExpose(evt.detail, 'authenticate', function(password) {
-        if (callbackCalled)
-          return;
-        callbackCalled = true;
-        callback(true, null, password);
-      });
+      // We don't handle password-only prompt, so just cancel it.

s/prompt/prompts/

diff -u b/dom/browser-element/BrowserElementPromptService.jsm b/dom/browser-element/BrowserElementPromptService.jsm
--- b/dom/browser-element/BrowserElementPromptService.jsm
+++ b/dom/browser-element/BrowserElementPromptService.jsm
@@ -98,8 +98,9 @@
   asyncPromptAuth: function asyncPromptAuth(channel, callback, context, level, authInfo) {
     debug("asyncPromptAuth");
 
-    // We don't authenticate proxies currently
-    if (authInfo.flags & Ci.nsIAuthInformation.AUTH_PROXY) {
+    // The cases that we don't support now.
+    if ((authInfo.flags & Ci.nsIAuthInformation.AUTH_PROXY)
+        && (authInfo.flags & Ci.nsIAuthInformation.ONLY_PASSWORD)) {

Nit: && on the line above.  (We do this everywhere in Gecko.)

Let's do this.
Attachment #657814 - Flags: review?(justin.lebar+bug) → review+
Comment on attachment 657825 [details] [diff] [review]
Test case

I'm sorry again for the wait here.
Attachment #657825 - Flags: review?(justin.lebar+bug) → review+
r+ by :smaug in comment 64. Just fix nit.
Attachment #656843 - Attachment is obsolete: true
Attachment #658744 - Flags: review+
r+ in comment 111, fix nit and comment wording.
Attachment #657814 - Attachment is obsolete: true
Attachment #658747 - Flags: review+
Whiteboard: [mentor=jlebar][lang=js][LOE:M] → [mentor=jlebar][lang=js][LOE:M][qa-]
Whiteboard: [mentor=jlebar][lang=js][LOE:M][qa-] → [mentor=jlebar][lang=js][LOE:M][qa-][WebAPI:P2]
Depends on: 789721
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: