462739 - Send cookies as appropriate with xpinstall requests

Status: RESOLVED FIXED

(Core Graveyard :: Installer: XPInstall Engine, defect)

Description

[Dave Townsend [:mossop]]

Attachment: patch rev 1

Spun off from bug 437174, I don't think the right fix for xpinstall is just to blanket always or never send cookies. We should be sending cookies as appropriate according to the page initiating the request (the page that called InstallTrigger.install). This patch does that by making our requests in the same load group as the original page. This allows the cookie service to correctly compare the host we are requesting from to the host of the original page and so send or not send cookies.

Comment 1

[Daniel Veditz [:dveditz]]

Comment on attachment 345937 [details] [diff] [review]
patch rev 1

r/sr=dveditz

Comment 2

[Dave Townsend [:mossop]]

Comment on attachment 345937 [details] [diff] [review]
patch rev 1

I would like to land this for 1.9.1 to allow people to install AMO sandboxed add-ons when they have third party cookies off.

Comment 3

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 345937 [details] [diff] [review]
patch rev 1

a191=beltzner

Comment 4

[Dave Townsend [:mossop]]

Attachment: patch for checkin

Comment 5

[Dave Townsend [:mossop]]

Attachment: patch for checkin

Comment 6

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/mozilla-central/rev/c99b74e60a17

Comment 7

[Daniel Veditz [:dveditz]]

This seems appropriate and useful to take in a 3.0.x release

Comment 8

[Daniel Veditz [:dveditz]]

Mossop: Sam says I can't block on this, but I would like to fix this in 1.9.0. Will the patches here apply to 1.9.0 or could you come up with a 1.9.0 version?

Comment 9

[Dave Townsend [:mossop]]

(In reply to comment #8)
> Mossop: Sam says I can't block on this, but I would like to fix this in 1.9.0.
> Will the patches here apply to 1.9.0 or could you come up with a 1.9.0 version?

We should not take this on 1.9.0.x until bug 473060 has been resolved. It is currently one of my top priorities but will still be early next week before I have a definite fix.

Comment 10

[Daniel Veditz [:dveditz]]

Comment on attachment 350490 [details] [diff] [review]
patch for checkin

next time, then.

Comment 11

[Dave Townsend [:mossop]]

Comment on attachment 350490 [details] [diff] [review]
patch for checkin

We can take this on branch now so long as we also take bug 473060

Comment 12

[Daniel Veditz [:dveditz]]

Where is the test that "in-testsuite+" refers to?

Comment 13

[Dave Townsend [:mossop]]

(In reply to comment #12)
> Where is the test that "in-testsuite+" refers to?

It was landed as part of bug 474763

Comment 14

[Daniel Veditz [:dveditz]]

Comment on attachment 350490 [details] [diff] [review]
patch for checkin

Approved for 1.9.0.8, a=dveditz for release-drivers

Comment 15

[Dave Townsend [:mossop]]

Landed on 1.9 branch:

Checking in xpinstall/src/nsXPInstallManager.cpp;
/cvsroot/mozilla/xpinstall/src/nsXPInstallManager.cpp,v  <--  nsXPInstallManager.cpp
new revision: 1.166; previous revision: 1.165
done
Checking in xpinstall/src/nsXPInstallManager.h;
/cvsroot/mozilla/xpinstall/src/nsXPInstallManager.h,v  <--  nsXPInstallManager.h
new revision: 1.48; previous revision: 1.47
done

Comment 16

[Al Billings [:abillings - ex-MoCo]]

The tests for this don't seem to exist in the 1.9.0 branch (from bug 474763). Is there a way for QA to verify this for 1.9.0.8?