1237834 - test_getauthenticationinfo.html pops up login dialog with e10s

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Andrew McCreight [:mccr8]]

When run with e10s, this test pops up an authentication dialog. When run without it, it does not.

This is similar to bug 1221320, though there's no XHR here, so it doesn't seem like it could be the exact same thing. It might be an issue with the test plugin. I don't know what is responsible for popping up that authentication dialog.

Comment 1

[Andrew McCreight [:mccr8]]

This might be an actual Gecko bug. I can't really tell.

Comment 2

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Ok, this is the only test outside of things in toolkit/components/passwordmgr/test that is using authenticate.sjs. All mochitests in toolkit/components/passwordmgr/test are marked skip for e10s. So I'm guessing this component just doesn't work with e10s. What's the next step here? Any plans to fix the password manager tests also? I didn't see an open bug for that yet...

Comment 3

[Andrew McCreight [:mccr8]]

(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #2)
> Ok, this is the only test outside of things in
> toolkit/components/passwordmgr/test that is using authenticate.sjs. All
> mochitests in toolkit/components/passwordmgr/test are marked skip for e10s.
> So I'm guessing this component just doesn't work with e10s. What's the next
> step here? Any plans to fix the password manager tests also? I didn't see an
> open bug for that yet...

In the e10s test spreadsheet, somebody marked all of those tests "Tests are finicky and will likely be involved in getting fixed up" and marked them for Manual QA. So I guess nobody is looking at that. But the sjs is just some fake server written in JS so I'd be a little surprised if it was the .sjs in particular broken in e10s. I haven't actually checked it, though.

Comment 4

[Andrew McCreight [:mccr8]]

There's also browser/base/content/test/general/authenticate.sjs which seems to be a similar file, and it is being used in a bc test browser_thumbnails_bg_no_auth_prompt.js that seems to be running with e10s so you could try that and see if it helps anything.

Comment 5

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Ok, authenticate.sjs isn't the issue, browser one does the same thing. Will continue looking.

Comment 6

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v1) - Make sure auth ident is set in both parent and child processes

The problem ended up being in nsIHTTPAuthManager. This is a service that is only used by tests in order to manipulate the auth cache. However, as far as I can tell in DXR, the only e10s test that hits this is test_getauthenticationinfo.html.

This patch is the short, hacky version of the fix. Basically, two auth checks happen. One in the plugin, which queries the content process for auth info, and one in HTTP network stuff, which happens in the parent process, and is what was popping the window, since we weren't adding to the auth cache there. I just added a chrome script to add the auth cache info in the parent process, so both checks are happy.

The actual fix would just be to make HTTPAuthCache an e10s compliant service that only exists on the parent process. That's a followup bug situation, though, as right now I'm just trying to get this un-oranged, and no other tests seem to be exhibiting this issue.

Comment 7

[Andrew McCreight [:mccr8]]

Comment on attachment 8719072 [details] [diff] [review]
Patch 1 (v1) - Make sure auth ident is set in both parent and child processes

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

::: dom/plugins/test/mochitest/mochitest.ini
@@ +3,4 @@
>  support-files =
>    307-xo-redirect.sjs
>    crashing_subpage.html
> +  file_authident.js

You also need to remove the skip-if for test_getauthenticationinfo.html in this file.

::: dom/plugins/test/mochitest/test_getauthenticationinfo.html
@@ +61,4 @@
>  SimpleTest.waitForExplicitFinish();
>  setTestPluginEnabledState(SpecialPowers.Ci.nsIPluginTag.STATE_ENABLED);
>  
> +// Authentication info is added twice here. In the non-e10s case, this does

Thanks for the comment.

@@ +61,5 @@
>  SimpleTest.waitForExplicitFinish();
>  setTestPluginEnabledState(SpecialPowers.Ci.nsIPluginTag.STATE_ENABLED);
>  
> +// Authentication info is added twice here. In the non-e10s case, this does
> +// nothing. In the e10s case, this is because we need it auth info both the

This sentence is a little mangled. "it auth info both" should be "auth info in both" maybe?

@@ +64,5 @@
> +// Authentication info is added twice here. In the non-e10s case, this does
> +// nothing. In the e10s case, this is because we need it auth info both the
> +// child process, that the plugin checks for auth validity, and the parent process,
> +// that the http network objects use.
> +// TODO: Clean this up once HTTPAuthManager is made e10s compliant

Is there a bug on file for this that you could refer to?

Comment 8

[Pulsebot]

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

Comment 9

[Carsten Book [:Tomcat]]

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

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/211c02192daf