Port test_prompt.html to the new per-window PB APIs

NEW
Unassigned

Status

()

Firefox
Private Browsing
6 years ago
5 years ago

People

(Reporter: Ehsan, Unassigned)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [leave open])

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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.)
(Reporter)

Comment 1

6 years ago
Removed this test from per-window PB builds: https://hg.mozilla.org/mozilla-central/rev/d5d64fa4d7c6
Assignee: nobody → andres
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.
(Reporter)

Comment 3

6 years ago
(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?
Created attachment 699658 [details] [diff] [review]
v1 (WIP)

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

Comment 9

6 years ago
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?
(Reporter)

Comment 11

6 years ago
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...
(Reporter)

Comment 12

6 years ago
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...
Blocks: 817477
(Reporter)

Updated

6 years ago
Depends on: 835904
(Reporter)

Comment 13

6 years ago
I filed bug 835904 for the js error here.  This seems to be a real regression.
Status: NEW → ASSIGNED
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) \
Attachment #699658 - Attachment is obsolete: true
Attachment #714466 - Flags: review?(ehsan)
(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.

Updated

6 years ago
Summary: Port test_prompt.html to the new per-tab PB APIs → Port test_prompt.html to the new per-window PB APIs
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
(Reporter)

Comment 18

6 years ago
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.
Attachment #714466 - Flags: review?(ehsan)
Created attachment 715531 [details] [diff] [review]
Patch v2

Patch without whitespaces for review.
Attachment #714466 - Attachment is obsolete: true
Attachment #715531 - Flags: review?(ehsan)
Created attachment 715535 [details] [diff] [review]
Patch v2 (for checkin)

Patch with whitespaces for checkin.
(Reporter)

Comment 21

6 years ago
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!
Attachment #715531 - Flags: review?(ehsan) → review+
(Reporter)

Comment 22

6 years ago
Landed the first part: https://hg.mozilla.org/integration/mozilla-inbound/rev/1d96b58d9d4b
Whiteboard: [leave open]
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.
(Reporter)

Comment 24

6 years ago
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
Created attachment 718055 [details] [diff] [review]
Part 2 - Migration

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?
Attachment #718055 - Flags: feedback?(ehsan)
(Reporter)

Comment 27

6 years ago
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.
Attachment #718055 - Flags: feedback?(ehsan) → feedback+

Updated

5 years ago
See Also: → bug 834836
Assignee: andres → nobody
Status: ASSIGNED → NEW
You need to log in before you can comment on or make changes to this bug.