1241557 - Make extensions/cookie mochitests work in e10s

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Blake Kaplan (:mrbkap) (inactive)]

They need to use a chrome script in order to access the cookie manager.

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix test_loadflags.html

Most tests in this directory use a common utils file. This is the odd one out since it uses an http observer.

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix the rest of the tests

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

I will land these patches squashed into a single patch.

Comment 4

[Andrew McCreight [:mccr8]]

Comment on attachment 8710567 [details] [diff] [review]
Fix test_loadflags.html

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

::: extensions/cookie/test/file_testloadflags.js
@@ +1,1 @@
> +var SCRIPT_URL = SimpleTest.getTestFileURL('file_testloadflags_chromescript.js');

const instead of var maybe?

@@ +16,1 @@
>             " loads: " + loads + " headers: " + headers);

nit: fix the indentation, please.

@@ +42,4 @@
>    // Listen for MessageEvents.
>    window.addEventListener("message", messageReceiver, false);
>  
> +  Promise.all([ prefSet, scriptReady ]).then(() => {

I just want to double check: the code in scriptReady doesn't depend on the pref, right? (I don't know what the pref does, I just noticed this is a change in behavior, as the "init" code would run with the pref set before.) I tried it locally to explicitly order prefSet to run after scriptReady and it looked okay.

::: extensions/cookie/test/file_testloadflags_chromescript.js
@@ +6,5 @@
> +  sendAsyncMessage("info", { str: String(s) });
> +}
> +
> +function ok(c, m) {
> +  sendAsyncMessage("ok", { c, m });

What does { c, m } do? Is that sort of like an array?

@@ +69,5 @@
> +
> +  info("we are going to remove these cookies");
> +  let count = 0;
> +  let list = cs.enumerator;
> +  while (list.hasMoreElements()) {

This chunk of code looks identical to getCookieCount(). Maybe you could factor it out?

Comment 5

[Andrew McCreight [:mccr8]]

Comment on attachment 8710569 [details] [diff] [review]
Fix the rest of the tests

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

::: extensions/cookie/test/file_chromecommon.js
@@ +3,5 @@
> +let cs = Cc["@mozilla.org/cookiemanager;1"]
> +           .getService(Ci.nsICookieManager2);
> +
> +addMessageListener("init", () => {
> +  cs.removeAll();

AFAICT, unlike in the previous patch, this cs.removeAll() could just be done in the top level of the chrome script, and then you could avoid the init back and forth.

@@ +9,5 @@
> +});
> +
> +addMessageListener("getCookieCountAndClear", () => {
> +  let count = 0;
> +  for(let list = cs.enumerator; list.hasMoreElements(); list.getNext())

nit: add a space between "for" and "(".

::: extensions/cookie/test/file_testcommon.js
@@ +1,1 @@
> +var SCRIPT_URL = SimpleTest.getTestFileURL("file_chromecommon.js");

const instead of var maybe.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Roll-up patch

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dcee6cb6f085

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Another cookie test

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Andrew McCreight [:mccr8] from comment #4)
> I just want to double check: the code in scriptReady doesn't depend on the
> pref, right? 

Right. It's fine for these to race.

> ::: extensions/cookie/test/file_testloadflags_chromescript.js
> What does { c, m } do? Is that sort of like an array?

That's a shorthand object literal, it's equivalent for { c: c, m: m }.

Comment 10

[Andrew McCreight [:mccr8]]

Comment on attachment 8710638 [details] [diff] [review]
Another cookie test

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

::: dom/html/test/test_non-ascii-cookie.html
@@ +50,5 @@
> +  is(cookie, document.cookie, "nsICookieManager should be consistent with document.cookie");
> +  var date1 = new Date();
> +  date1.setTime(0);
> +  document.cookie = newCookie + 'def=;expires=' + date1.toGMTString();
> +  gScript.destroy();

Do you actually need to destroy these scripts explicitly?

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Andrew McCreight [:mccr8] from comment #10)
> Do you actually need to destroy these scripts explicitly?

It looks like destroy only prevents spurious messages from coming from the script. I figure it can't hurt to call it.

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Another test that already passes

bz has an open rs for tests that already pass.

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: An easy test to fix

This one just sets prefs directly and doesn't wait for them to take effect.

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8ed3a7be233e91bbfad20a3dbf5551b5ffcbf6e0
Bug 1241557 - Make cookie tests pass in e10s. r=mccr8

https://hg.mozilla.org/integration/mozilla-inbound/rev/aeb7f61db88b84e300c24859c44da7fa0ef80844
Bug 1241557 - Don't try to set prefs directly. r=wchen

Comment 15

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

Backed out for build bustage: https://treeherder.mozilla.org/logviewer.html#?job_id=20242824&repo=mozilla-inbound

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

 15:27:48     INFO -  From dist/private: Kept 0 existing; Added/updated 0; Removed 156 files and 2 directories.
 15:27:51     INFO -  From dist/xpi-stage: Kept 12 existing; Added/updated 0; Removed 486 files and 107 directories.
 15:27:53     INFO -  From dist/idl: Kept 1146 existing; Added/updated 0; Removed 0 files and 0 directories.
 15:27:59     INFO -  From dist/include: Kept 4700 existing; Added/updated 1; Removed 187 files and 6 directories.
 15:28:12     INFO -  From dist/bin: Kept 1768 existing; Added/updated 0; Removed 2979 files and 342 directories.
 15:28:28     INFO -  Traceback (most recent call last):
 15:28:28     INFO -    File "/tools/python/lib/python2.7/runpy.py", line 162, in _run_module_as_main
 15:28:28     INFO -      "__main__", fname, loader, pkg_name)
 15:28:28     INFO -    File "/tools/python/lib/python2.7/runpy.py", line 72, in _run_code
 15:28:28     INFO -      exec code in run_globals
 15:28:28     INFO -    File "/builds/slave/m-in-m64-d-0000000000000000000/build/src/python/mozbuild/mozbuild/action/process_install_manifest.py", line 103, in <module>
 15:28:28     INFO -      main(sys.argv[1:])
 15:28:28     INFO -    File "/builds/slave/m-in-m64-d-0000000000000000000/build/src/python/mozbuild/mozbuild/action/process_install_manifest.py", line 94, in main
 15:28:28     INFO -      defines=args.defines)
 15:28:28     INFO -    File "/builds/slave/m-in-m64-d-0000000000000000000/build/src/python/mozbuild/mozbuild/action/process_install_manifest.py", line 62, in process_manifest
 15:28:28     INFO -      remove_empty_directories=remove_empty_directories)
 15:28:28     INFO -    File "/builds/slave/m-in-m64-d-0000000000000000000/build/src/python/mozbuild/mozpack/copier.py", line 384, in copy
 15:28:28     INFO -      if f.copy(destfile, skip_if_older):
 15:28:28     INFO -    File "/builds/slave/m-in-m64-d-0000000000000000000/build/src/python/mozbuild/mozpack/files.py", line 309, in copy
 15:28:28     INFO -      raise ErrorMessage('Symlink target path does not exist: %s' % self.path)
 15:28:28     INFO -  mozpack.errors.ErrorMessage: Symlink target path does not exist: /builds/slave/m-in-m64-d-0000000000000000000/build/src/dom/html/test/file_cookiemanager.js
 15:28:28     INFO -  make[4]: *** [install-_tests] Error 255
 15:28:28     INFO -  make[3]: *** [pre-export] Error 2
 15:28:28     INFO -  make[2]: *** [default] Error 2
 15:28:28     INFO -  make[1]: *** [realbuild] Error 2
 15:28:28     INFO -  make: *** [build] Error 2
 15:28:29     INFO -  268 compiler warnings present.
 15:28:30    ERROR - Return code: 2
 15:28:30  WARNING - setting return code to 2
 15:28:30    FATAL - 'mach build' did not run successfully. Please check log for errors.
 15:28:30    FATAL - Running post_fatal callback...
 15:28:30    FATAL - Exiting -1
 15:28:30     INFO - Running post-action listener: influxdb_recording_post_action
 15:28:30     INFO - Resetting dropped connection: goldiewilson-onepointtwentyone-1.c.influxdb.com
 /builds/slave/m-in-m64-d-0000000000000000000/build/venv/lib/python2.7/site-packages/requests/packages/urllib3/util/ssl_.py:90: InsecurePlatformWarning: A true SSLContext object is not available. This prevents urllib3 from configuring SSL appropriately and may cause certain SSL connections to fail. For more information, see https://urllib3.readthedocs.org/en/latest/security.html#insecureplatformwarning.
   InsecurePlatformWarning
 15:28:30     INFO - Running post-action listener: record_mach_stats
 15:28:30     INFO - No build_resources.json found, not logging stats
 15:28:30     INFO - Running post-run listener: _summarize
 15:28:30    ERROR - # TBPL FAILURE #

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/afb7190f14d97225c8a6465dfe763411e58a4d65
Bug 1241557 - Make cookie tests pass in e10s. r=mccr8

https://hg.mozilla.org/integration/mozilla-inbound/rev/2f5e4d2d6f3588c5f001afc8b7299a1a016fa904
Bug 1241557 - Don't try to set prefs directly. r=wchen

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/afb7190f14d9
https://hg.mozilla.org/mozilla-central/rev/2f5e4d2d6f35