http://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/test/test_prompt.html In order to port this test, the file needs to be copied to the same directory (perhaps with "_perwindowpb" appended to its file name), and then instead of setting privateBrowsingEnabled, we need to open a new private browsing window and then run the test on that window. Note that the original test should only be added to the list of test files in Makefile.in ifndef MOZ_PER_WINDOW_PRIVATE_BROWSING, and the new test file should be added to the list with the reverse condition. (Note that only the block around |if (pb)| needs porting.)

Removed this test from per-window PB builds: https://hg.mozilla.org/mozilla-central/rev/d5d64fa4d7c6

I have been trying to port this test, but the test fails without any change (in both per window PB and global PB). The test fails when calling: > isOk = prompter2.promptAuth(proxyChannel, level, proxyAuthinfo); JavaScript error: file:///.../Mozilla/Central/obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/components/nsLoginManagerPrompter.js, line 577: NS_ERROR_XPC_JS_THREW_STRING: 'proxy auth needs nsIProxyInfo' when calling method: [nsIPromptService2::promptAuth] I'll keep researching, since it needs to be fix before is ported. If you have any suggestions, please let me know.

(In reply to Andres Hernandez [:andreshm] from comment #2) > I have been trying to port this test, but the test fails without any change > (in both per window PB and global PB). > > The test fails when calling: > > > isOk = prompter2.promptAuth(proxyChannel, level, proxyAuthinfo); > > JavaScript error: > file:///.../Mozilla/Central/obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/ > components/nsLoginManagerPrompter.js, line 577: > NS_ERROR_XPC_JS_THREW_STRING: 'proxy auth needs nsIProxyInfo' when calling > method: [nsIPromptService2::promptAuth] > > I'll keep researching, since it needs to be fix before is ported. > If you have any suggestions, please let me know. Dolske might know.

(In reply to Andres Hernandez [:andreshm] from comment #2) > I have been trying to port this test, but the test fails without any change > (in both per window PB and global PB). You mean it's failing for you for a clean, plain, unmodified mozilla-central build? That would imply something's wrong on your end. I'm not really sure what would cause that... As a wild guess, system-wide proxy setting?

Yes, with a clean unmodified mozilla-central build, the test ends correctly, running all the test suite with: TEST_PATH=toolkit/components/passwordmgr/test/ make -C obj-ff-dbg mochitest-plain But, if you add a dump/info log to the test you will see that it never reaches the test 509 or further, because it fails in test 508 of test_prompt.html. Specifically in this line: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/test/test_prompt.html?force=1#1064 Not sure why the test doesn't fail and continues.

I've no idea what might be wrong. Maybe something in the tree broke this test when it was disabled for the per-window PB landing? That would be sadmaking. :( You'll need to debug it.

More details about the issue. --- FROM test_prompt.html --- // I'm cheating a bit here... We should probably do some magic foo to get // something implementing nsIProxiedProtocolHandler and then call // NewProxiedChannel(), so we have something that's definately a proxied // channel. But Mochitests use a proxy for a number of hosts, so just // requesting a normal channel will give us a channel that's proxied. var proxyChannel = ioService.newChannel(proxiedHost, null, null); var level = Ci.nsIAuthPrompt2.LEVEL_NONE; // ===== test 508 ===== proxyAuthinfo.username = ""; proxyAuthinfo.password = ""; proxyAuthinfo.realm = "Proxy Realm"; proxyAuthinfo.flags = Ci.nsIAuthInformation.AUTH_PROXY; isOk = prompter2.promptAuth(proxyChannel, level, proxyAuthinfo); -- END --- When Prompter::promptAuth(aChannel, aLevel, aAuthInfo) -> this._getAuthTarget(aChannel, aAuthInfo) are executed, it throws an error 'proxy auth needs nsIProxyInfo' because aChannel.proxyInfo returns null, the test doesn't carry on and the next test kicks in without any errors / warnings. See http://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/nsLoginManagerPrompter.js#516 The nsIProxiedChannel inteface doesn't provide any interface to set the proxy info. See http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsIProxiedChannel.idl#16 Any suggestions how to fix this?

I've added a try/catch block in test 508 and you should see the error when you run it

Hmm, so I debugged this a bit. When the error happens, nsHttpChannel::GetProxyInfo <http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#4581> is called. There, mConnectionInfo is null, and mProxyInfo is null as well, so we end up returning null, and things break in our face. I double-checked, and the global PB version of this test succeeds on the Birch branch: <https://tbpl.mozilla.org/php/getParsedLog.php?id=18679551&tree=Birch&full=1>. Patrick, can you think of what's going wrong here? I'd appreciate your help, as I'm not very familiar with this code. Note that this exception is reproducible on trunk even without this patch if you just edit the makefile to make it run test_prompt.html.

(In reply to :Ehsan Akhgari from comment #9) > Hmm, so I debugged this a bit. When the error happens, > nsHttpChannel::GetProxyInfo > <http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/ > nsHttpChannel.cpp#4581> is called. There, mConnectionInfo is null, and > mProxyInfo is null as well, so we end up returning null, and things break in > our face. I haven't looked in detail at the test yet, so I'll just talk in generalities. proxyinfo is established after channel->asyncopen() is called but before the http-on-modify-request observer is called (and it can be changed by those observers) before any connection is made. So its my guess the test tries to look at it "too early". Proxy resolution can block the thread (pac files can do DNS for instance), so it needs to be done asynchronously and that's really the first place in the channel creation chain that goes async. does that help?

Raymond, did you get a chance to look at comment 10? One thing to see is whether this test passes if you add |export MOZ_PER_WINDOW_PRIVATE_BROWSING=0| to your mozconfig, and therefore, disable per-window private browsing...

Over in bug 817477, I'm preparing to remove support for global private browsing altogether. This is one of the last remaining pieces that I would like us to fix before that bug lands...

I filed bug 835904 for the js error here. This seems to be a real regression.

Fixed test and code indentation. Should I changed the following line in the makefile? > $(filter disabled-for-intermittent-failures--bug-835904, test_prompt.html) \

(In reply to Andres Hernandez [:andreshm] from comment #14) > Created attachment 714466 [details] [diff] [review] > Patch v1 > > Fixed test and code indentation. > > Should I changed the following line in the makefile? > > > $(filter disabled-for-intermittent-failures--bug-835904, test_prompt.html) \ before enabling the test, please take note of https://bugzilla.mozilla.org/show_bug.cgi?id=835904#c8 in particular that test still does not pass (at least for me locally) - but in a much different place and for reasons I believe are unrelated to the problems fixed by 835904. It needs to find an owner familiar with the topic area.

Patrick, the patch on comment 14 fixes the popupNotifications issue and it now passes locally.

All green on try (without the filter in the makefile): https://tbpl.mozilla.org/?tree=Try&rev=f298790364e7

Comment on attachment 714466 [details] [diff] [review] Patch v1 Review of attachment 714466 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, Andres, but this is a pain to review because of all of the indentation changes. Can you please move the whitespace changes to another patch, or at least submit a diff -w patch for review? Thanks! Also, please re-enable the test in the Makefile in the next version of the patch.

Patch without whitespaces for review.

Patch with whitespaces for checkin.

Comment on attachment 715531 [details] [diff] [review] Patch v2 Review of attachment 715531 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, this looks good, and does the job as far as re-enabling test_prompt.html is concerned, but it doesn't port the PB test related to test_prompt.html, specifically the part of the test removed in bug 817477 <https://hg.mozilla.org/mozilla-central/rev/6aaf13ffc716#l221.34>. Can you please submit another patch for those pieces? You should be able to use a different prompter object constructed from a private window for that part of the test. Thanks!

Landed the first part: https://hg.mozilla.org/integration/mozilla-inbound/rev/1d96b58d9d4b

Backed out because of frequent failures in test_prompt.html on Linux: https://hg.mozilla.org/integration/mozilla-inbound/rev/4e6834c859a8 Example failure log: https://tbpl.mozilla.org/php/getParsedLog.php?id=19935043&tree=Mozilla-Inbound Before re-enabling this test you should make sure to trigger several test runs on Try. Also, note that it's mochitest-plain, not mochitest-bc.

Relanded while disabling the test on Linux. Note that this test was previously unstable and disabled on Linux too: https://hg.mozilla.org/integration/mozilla-inbound/rev/e7d1e1d55534

This is the migration part. It runs fine, but the timeLastUsed check is currently failing, since a modifyLogin event is being fired in the private window, that changes the timeLastUsed value. Not sure why, any ideas?

Comment on attachment 718055 [details] [diff] [review] Part 2 - Migration Review of attachment 718055 [details] [diff] [review]: ----------------------------------------------------------------- Looks good overall, but I don't have any idea about the test failure off hand.