Closed Bug 1361675 Opened 7 years ago Closed 7 years ago

Cookie import test fails on Win 10 (taskcluster)

Categories

(Firefox :: Migration, defect)

Product:
Firefox
Component:
Migration
Platform:
x86_64
Windows 10
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: grenade, Assigned: mak)

References

Details

Attachments

(1 file, 1 obsolete file)

Bug 1361675 - Cookie import test fails on Win 10.
59 bytes, text/x-review-board-request
Gijs
: review+
Details 
in bug 1154294, comment 2 & 3, there is some speculation about cookie import failures on windows 10. those have now materialised.

there is an example here in the xpcshell chunk 1 tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=43ed0c81835ac82c0811dd19882c51f42321a0b5&group_state=expanded&filter-searchStr=windows10&selectedJob=96185813
(In reply to Rob Thijssen (:grenade - EEST) from comment #0)
> in bug 1154294, comment 2 & 3, there is some speculation about cookie import
> failures on windows 10. those have now materialised.
> 
> there is an example here in the xpcshell chunk 1 tests:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=43ed0c81835ac82c0811dd19882c51f42321a0b5&group_state=e
> xpanded&filter-searchStr=windows10&selectedJob=96185813

I don't think this is failing for the reason bug 1154294 comment 2 imagined, though (ie the wininet dll import failing). Lots of the other logging seems to imply this is working fine, but the importer is somehow broken. Maybe the format of cookie files changed in win10?

Marco, do you have cycles to take a look at this?
Flags: needinfo?(mak77)
The cookies from Windows 8 on are stored in a new folder, for example in my case:
C:\Users\marco\AppData\Local\Microsoft\Windows\INetCookies
Luckily our code does the right thing from Services.dirsvc.get("CookD", Ci.nsIFile).path. This is good.

But then we do this:
        // Check if UAC is enabled.
        if (Services.appinfo.QueryInterface(Ci.nsIWinAppHelper).userCanElevate) {
          cookiesFolder.append("Low");

On my system Services.appinfo.QueryInterface(Ci.nsIWinAppHelper).userCanElevate returns true, but cookies are not in the Low folder. The above code is indeed only valid on Windows 7 or earlier.

Inside the INetCookies folder, there is an additional "PrivacIE/Low" set of folders, but these are no more related to the old Low folder. They are for the InPrivate Filtering feature, and likely we don't care about those.

So, first thing to do, seems to be making the "Low" appending code only affect Windows 7 or earlier.

Second issue appears to be here:
http://searchfox.org/mozilla-central/rev/6580dd9a4eb0224773897388dba2ddf5ed7f4216/browser/components/migration/MSMigrationUtils.jsm#516

We only consider .txt files, but now cookie files, even if they are still txt files, have a .cookie extension.
At first glance the format doesn't seem to have changed, so it should be just matter of accepting a further extension and the import code should work.

Do you have resources to work on these changes and check if the test passes, and then I can review the patch, or should I cut some time off other priorities to make a patch?
Flags: needinfo?(mak77) → needinfo?(rthijssen)
I will take a stab at the patch!
Flags: needinfo?(rthijssen)
::mak, do you know where Services.cookies.countCookiesFromHost is defined?

I am failing here now:
https://dxr.mozilla.org/mozilla-central/source/browser/components/migration/tests/unit/test_IE_cookies.js#105

and I want to make sure this is counting cookies correctly (i.e. with a .cookie extension).
Flags: needinfo?(mak77)
(In reply to Joel Maher ( :jmaher) from comment #4)
> ::mak, do you know where Services.cookies.countCookiesFromHost is defined?
> 
> I am failing here now:
> https://dxr.mozilla.org/mozilla-central/source/browser/components/migration/
> tests/unit/test_IE_cookies.js#105
> 
> and I want to make sure this is counting cookies correctly (i.e. with a
> .cookie extension).

This lives on the cookie service. It's counting the cookies that have been imported. The extension shouldn't matter anymore at that point - we store cookies in a sqlite database, not in files. The files are what IE/Edge/Windows use.
(In reply to Joel Maher ( :jmaher) from comment #4)
> ::mak, do you know where Services.cookies.countCookiesFromHost is defined?
> 
> I am failing here now:
> https://dxr.mozilla.org/mozilla-central/source/browser/components/migration/
> tests/unit/test_IE_cookies.js#105

If you fail there, something is wrong yet, that just checks that we were able to import the cookie. Feel free to post the patch and we can have a look at it.
Flags: needinfo?(mak77)
Attached patch support edge in cookie migration (obsolete) — — Splinter Review 
0:02.91 TEST_STATUS: Thread-7  PASS true == true
0:02.92 TEST_STATUS: Thread-7  PASS Added a persistent IE cookie: testvalue; expires=Wed, 10 May 2017 17:21:46 GMT - true == true
0:02.93 TEST_STATUS: Thread-7  PASS Found the added persistent IE cookie - true == true
0:02.93 LOG: Thread-7 INFO "Found cookie: testcookie=testvalue"
0:02.93 TEST_STATUS: Thread-7  PASS Found the expected cookie - "testcookie=testvalue" == "testcookie=testvalue"
0:02.98 TEST_STATUS: Thread-7  PASS There are no cookies initially - 0 == 0
0:02.98 TEST_STATUS: Thread-7  PASS Resource supported by migrator - true == true
0:02.98 LOG: Thread-7 INFO (xpcshell/head.js) | test run_next_test 0 finished (2)
0:04.47 TEST_STATUS: Thread-7  FAIL Migrated the expected number of cookies - 0 == 1
   c:/Users/elvis/mozilla-inbound/obj-i686-pc-mingw32/_tests/xpcshell/browser/components/migration/tests/unit/test_IE_cookies.js:null:105
   _run_next_test@c:\Users\elvis\mozilla-inbound\testing\xpcshell\head.js:1554:9
   run@c:\Users\elvis\mozilla-inbound\testing\xpcshell\head.js:703:9
   _do_main@c:\Users\elvis\mozilla-inbound\testing\xpcshell\head.js:211:5
   _execute_test@c:\Users\elvis\mozilla-inbound\testing\xpcshell\head.js:542:5
   @-e:1:1
0:04.47 LOG: Thread-7 INFO exiting test
0:04.47 LOG: Thread-7 ERROR Unexpected exception 2147500036
ndefined
0:04.47 LOG: Thread-7 INFO exiting test
0:04.47 TEST_STATUS: Thread-7  PASS Expired the IE cookie - true == true
0:04.47 TEST_STATUS: Thread-7  PASS The cookie has been properly removed - true == true
0:04.58 TEST_END: Thread-7 Harness FAIL, expected PASS. Subtests passed 8/9. Unexpected 1
Based on the patch, you didn't address the first part of comment 2, that is on Windows 8.0+ we should NOT append the "Low" string to the cookies path.
Also, it's not just Windows 10, the problem is from Windows 8 on from what I could gather, so the checks should be for version 6.1
Assignee: nobody → jmaher
Status: NEW → ASSIGNED
I did originally, but in tracing on win10, there is a different code path for 'edge' by calling cookieFolders():
https://dxr.mozilla.org/mozilla-central/source/browser/components/migration/MSMigrationUtils.jsm?q=path%3AMSMigrationUtils.jsm&redirect_type=single#482

that decisions is made here:
https://dxr.mozilla.org/mozilla-central/source/browser/components/migration/MSMigrationUtils.jsm?q=path%3AMSMigrationUtils.jsm&redirect_type=single#509


This test is passing on windows 8, so I would be reluctant to change that behavior, but would be happy to and will verify on try.
test_IE_cookies is expected to test IE, not Edge. if it's testing Edge, it's doing the wrong thing, Edge should have its own test.
odd, now this test is passing locally without any patches :(
unfortunately this fails on try still, so I need to figure out how to reproduce this again locally.
Try clearing cookies both in IE and Edge. I could look at this in a couple days, when other priorities are out the way.
hm, I could only find Win 10 ASAN Tc-X tests on treeherder, I'm not sure how to run the tests on the other Win10 boxes?
Flags: needinfo?(jmaher)
ok, found them.
Flags: needinfo?(jmaher)
Attachment #8865524 - Attachment is obsolete: true
stealing, hope you don't mind.
Assignee: jmaher → mak77
Comment on attachment 8867126 [details]
Bug 1361675 - Cookie import test fails on Win 10.

https://reviewboard.mozilla.org/r/138722/#review142094

::: browser/components/migration/MSMigrationUtils.jsm:517
(Diff revision 2)
>        for (let folder of folders) {
>          let entries = folder.directoryEntries;
>          while (entries.hasMoreElements()) {
>            let entry = entries.getNext().QueryInterface(Ci.nsIFile);
>            // Skip eventual bogus entries.
> -          if (!entry.isFile() || !/\.txt$/.test(entry.leafName))
> +          if (!entry.isFile() || !/(\.cookie|\.txt)$/.test(entry.leafName))

super nitty nit: `/\.(cookie|txt)$/` is 1 character less repetition. :-)

Thanks for chasing this!
Attachment #8867126 - Flags: review?(gijskruitbosch+bugs) → review+
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/41c4e9a2f5a5
Cookie import test fails on Win 10. r=Gijs
https://hg.mozilla.org/mozilla-central/rev/41c4e9a2f5a5
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: