1095018 - Forward marionette's cookie interactions to the parent for compatibility with e10s

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Chris Manchester (limited bugmail, email directly)]

test_cookies.py fails with e10s turned on. According to :mconley in #e10s, our best bet will be to forward all our interactions with the cookie manager to the parent.

Comment 1

[Chris Manchester (limited bugmail, email directly)]

Attachment: Forward marionette's cookie interactions to the parent process for compatibility with e10s

e10s on:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d893ec5de843
e10s off:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=86579bf739a7

Comment 2

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: bz://1095018/chmanchester

Comment 3

[Chris Manchester (limited bugmail, email directly)]

/r/207 - Bug 1095018 - Forward marionette's cookie interactions to the parent process for compatibility with e10s.

Pull down this commit:

hg pull review -r aec5ba1434c06daecbb2b5b20b0fef167ffbc9f3

Comment 4

[Chris Manchester (limited bugmail, email directly)]

/r/207 - Bug 1095018 - Forward marionette's cookie interactions to the parent process for compatibility with e10s.

Pull down this commit:

hg pull review -r aec5ba1434c06daecbb2b5b20b0fef167ffbc9f3

Comment 5

[Andreas Tolfsen ❲:ato❳]

https://reviewboard.mozilla.org/r/205/#review167

Are the failures in e10s-enabled try expected? https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d893ec5de843

::: testing/marionette/marionette-listener.js
(Diff revision 1)
> -  let currentPath = location.pathname;
> +  let result = sendSyncMessage("Marionette:getVisibleCookies", {value: [currentPath,

Break line

::: testing/marionette/marionette-listener.js
(Diff revision 1)
> -
> -  let cookieManager;
> -  try {
> +  let cookies = getVisibleCookies(curFrame.location);
> +  if (typeof cookies == "undefined") {
> +    sendError("Error retrieving cookies", 13, null, msg.json.command_id);
> -    // Retrieving the cookie manager fails with e10s enabled.
> -    cookieManager = Cc['@mozilla.org/cookiemanager;1'].
> -                      getService(Ci.nsICookieManager);
> -  } catch (ex) {
> -    sendError("Error retrieving cookie manager: " + ex, 13, null, msg.json.command_id);
>      return;

Same as above.

::: testing/marionette/marionette-listener.js
(Diff revision 1)
> -  let cookieManager;
> -  try {
> -    // Retrieving the cookie manager fails with e10s enabled.
> -    cookieManager = Cc['@mozilla.org/cookiemanager;1'].
> -                      getService(Ci.nsICookieManager);
> +  let cookies = getVisibleCookies(curFrame.location);
> +  if (typeof cookies == "undefined") {
> +    sendError("Error retrieving cookies", 13, null, msg.json.command_id);
> +    return;
> +  }

Same as above.

::: testing/marionette/marionette-server.js
(Diff revision 1)
> +        Services.cookies.add(cookieToAdd.domain, cookieToAdd.path, cookieToAdd.name,

This previously used the cookieManager, like `Marionette:getVisibleCookies` and `Marionette:deleteCookie` does now.  Is `Services.cookies.add` correct?

::: testing/marionette/marionette-listener.js
(Diff revision 1)
>    var cookies = getVisibleCookies(curFrame.location);
>    if (typeof cookies == "undefined") {
> -    sendError("Error retrieving cookie manager", 13, null, msg.json.command_id);
> +    sendError("Error retrieving cookies", 13, null, msg.json.command_id);
>      return;
>    }

This will always return an empty array now.  It returned undefined when getVisibleCookies had a try...catch for importing the cookie manager.

I read your patch as if the cookie manager will always be present?  If that holds true we can remove this type check altogether.

Comment 6

[Chris Manchester (limited bugmail, email directly)]

https://reviewboard.mozilla.org/r/205/#review181

They are. Bug 1092223 tracks progress.

> This previously used the cookieManager, like `Marionette:getVisibleCookies` and `Marionette:deleteCookie` does now.  Is `Services.cookies.add` correct?

Services.cookies is a reference to a lazy getter for nsICookieManager2.

> This will always return an empty array now.  It returned undefined when getVisibleCookies had a try...catch for importing the cookie manager.
> 
> I read your patch as if the cookie manager will always be present?  If that holds true we can remove this type check altogether.

Right, I need to update this. I'm expecting the cookie manager will always be present in the parent.

Comment 7

[Chris Manchester (limited bugmail, email directly)]

/r/207 - Bug 1095018 - Forward marionette's cookie interactions to the parent process for compatibility with e10s.;r=ato
/r/403 - Fixes based on review feedback.

Pull down these commits:

hg pull review -r 0e6fd0b99d318c8fc83a1900b12fc83d3d326300

Comment 8

[Chris Manchester (limited bugmail, email directly)]

/r/207 - Bug 1095018 - Forward marionette's cookie interactions to the parent process for compatibility with e10s.;r=ato
/r/403 - Fixes based on review feedback.

Pull down these commits:

hg pull review -r 0e6fd0b99d318c8fc83a1900b12fc83d3d326300

Comment 9

[Andreas Tolfsen ❲:ato❳]

https://reviewboard.mozilla.org/r/205/#review183

Comment 10

[Chris Manchester (limited bugmail, email directly)]

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

Comment 11

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

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

Comment 13

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: Fixes based on review feedback.

Comment 14

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: Bug 1095018 - Forward marionette's cookie interactions to the parent process for compatibility with e10s.;r=ato