Last Comment Bug 421622 - XMLHttpRequest from chrome content clears Referer header
: XMLHttpRequest from chrome content clears Referer header
Status: RESOLVED FIXED
: fixed1.8.1.14, fixed1.8.1.15, regression
Product: Core
Classification: Components
Component: XML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla1.9beta5
Assigned To: Manish Singh
:
Mentors:
Depends on:
Blocks: 403168
  Show dependency treegraph
 
Reported: 2008-03-07 16:28 PST by Manish Singh
Modified: 2008-04-07 15:56 PDT (History)
12 users (show)
samuel.sidler+old: blocking1.8.1.13-
dveditz: blocking1.8.1.14+
dveditz: blocking1.8.1.15+
mattwillis: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Don't set referrer for code from the system principal (939 bytes, patch)
2008-03-07 16:28 PST, Manish Singh
jonas: review+
jonas: superreview+
mbeltzner: approval1.9+
Details | Diff | Review
Don't set referrer for code from the system principal - 1.8.1 branch version (1.31 KB, patch)
2008-03-13 13:04 PDT, Manish Singh
jonas: review+
samuel.sidler+old: approval1.8.1.13-
dveditz: approval1.8.1.14+
dveditz: approval1.8.1.15+
Details | Diff | Review
Mochitest testcase (5.19 KB, patch)
2008-03-15 17:51 PDT, Manish Singh
no flags Details | Diff | Review
Incorporated suggestions from Waldo for testcase code (5.13 KB, patch)
2008-03-17 16:20 PDT, Manish Singh
no flags Details | Diff | Review
rev0 - add additional makefiles to toolkit-makefiles.sh (861 bytes, patch)
2008-03-17 16:32 PDT, Matthew (lilmatt) Willis
benjamin: review+
Details | Diff | Review

Description Manish Singh 2008-03-07 16:28:30 PST
Created attachment 308077 [details] [diff] [review]
Don't set referrer for code from the system principal

When using setRequestHeader to set the Referer header on an XHR from a JS component, it doesn't work because the code in send() unconditionally resets it. This used to work in Gecko 1.8.1 at least, so this is a regression.

Attached patch fixes this, by changing to code to not touch the Referer header if called from chromeland.
Comment 1 Manish Singh 2008-03-11 18:59:25 PDT
And this is now regressed on 1.8.1 branch, due to what landed for bug #403168, at least according to CVS logs.
Comment 2 Matthew (lilmatt) Willis 2008-03-12 09:59:56 PDT
Adding regression keyword and requesting b1.8.1.13? to at least get this looked at.
Comment 3 Mike Beltzner [:beltzner, not reading bugmail] 2008-03-12 22:41:10 PDT
Comment on attachment 308077 [details] [diff] [review]
Don't set referrer for code from the system principal

a1.9=beltzner
Comment 4 Matthew (lilmatt) Willis 2008-03-13 04:43:51 PDT
Patch checked in on trunk.
new revision: 1.226; previous revision: 1.225

Awaiting branch version of patch.
Comment 5 Daniel Veditz [:dveditz] 2008-03-13 11:10:03 PDT
We already have 2.0.0.13 release candidate builds. What does this regression mean in practical terms -- is it a stop-ship bug? Seems like a glitch we can just fix in the next version.

Does this patch work on the branch? We can't even consider it for a respin if we don't have a patch ready to go.
Comment 6 Reed Loden [:reed] (use needinfo?) 2008-03-13 11:34:55 PDT
Fixed on trunk means this is FIXED.
Comment 7 Manish Singh 2008-03-13 12:09:46 PDT
(In reply to comment #5)
> We already have 2.0.0.13 release candidate builds. What does this regression
> mean in practical terms -- is it a stop-ship bug? Seems like a glitch we can
> just fix in the next version.

There may be extensions that rely on be able to set the referer header. Flock code does, which is why we noticed this regression, so this isn't necessarily theoretical.

> Does this patch work on the branch? We can't even consider it for a respin if
> we don't have a patch ready to go.

This patch doesn't apply to branch. I have a patch for branch, which I will attach shortly, after some testing.
Comment 8 Manish Singh 2008-03-13 13:01:31 PDT
Verified working on latest tinderbuild.
Comment 9 Manish Singh 2008-03-13 13:04:00 PDT
Created attachment 309196 [details] [diff] [review]
Don't set referrer for code from the system principal - 1.8.1 branch version

This is the patch for branch. I had to implement IsSystemPrincipal here, in terms of the 1.8.1 nsIScriptSecurityManager interface, which means getting the system principal and comparing the pointers.
Comment 10 Matthew (lilmatt) Willis 2008-03-13 15:22:02 PDT
Comment on attachment 309196 [details] [diff] [review]
Don't set referrer for code from the system principal - 1.8.1 branch version

Requesting a1.8.1.13?

If you're not respinning 13, then please assume a1.8.1.14?
Comment 11 Reed Loden [:reed] (use needinfo?) 2008-03-14 11:55:42 PDT
Why did you reopen this bug? It's fixed on trunk.
Comment 12 Reed Loden [:reed] (use needinfo?) 2008-03-14 12:03:12 PDT
Branch fixes are tracked with keywords, not bug resolution. Once a bug has been fixed on the trunk, it is resolved as FIXED and not left open pending branch landing.
Comment 13 Samuel Sidler (old account; do not CC) 2008-03-15 16:44:23 PDT
Olli, do you have any comment as to whether we should take this for 1.8.1.13? Do you think this is a serious enough regression from your patch?
Comment 14 Manish Singh 2008-03-15 17:51:18 PDT
Created attachment 309706 [details] [diff] [review]
Mochitest testcase
Comment 15 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2008-03-16 08:27:18 PDT
IMO we really should try to avoid all the regressions, so if we now have the
fix, we should take it. Even possibly breaking extensions is bad.
Comment 16 Manish Singh 2008-03-17 16:20:39 PDT
Created attachment 310112 [details] [diff] [review]
Incorporated suggestions from Waldo for testcase code
Comment 17 Matthew (lilmatt) Willis 2008-03-17 16:32:32 PDT
Created attachment 310122 [details] [diff] [review]
rev0 - add additional makefiles to toolkit-makefiles.sh

The test code adds a Makefile, and the content/base/test dir was missing
Comment 18 Benjamin Smedberg [:bsmedberg] 2008-03-18 09:38:13 PDT
Comment on attachment 310122 [details] [diff] [review]
rev0 - add additional makefiles to toolkit-makefiles.sh

trivial changes to *makefiles.sh do not need review
Comment 19 Matthew (lilmatt) Willis 2008-03-18 11:23:53 PDT
Testcase and toolkit-makefiles.sh landed on trunk
Comment 20 Samuel Sidler (old account; do not CC) 2008-03-26 11:04:00 PDT
1.8.1.13 is shipped.
Comment 21 Daniel Veditz [:dveditz] 2008-03-26 11:56:10 PDT
Comment on attachment 309196 [details] [diff] [review]
Don't set referrer for code from the system principal - 1.8.1 branch version

approved for 1.8.1.14, a=dveditz for release-drivers
Comment 22 Reed Loden [:reed] (use needinfo?) 2008-03-28 23:55:33 PDT
MOZILLA_1_8_BRANCH:

Checking in content/base/src/nsXMLHttpRequest.cpp;
/cvsroot/mozilla/content/base/src/nsXMLHttpRequest.cpp,v  <--  nsXMLHttpRequest.cpp
new revision: 1.156.2.13; previous revision: 1.156.2.12
done
Comment 23 Daniel Veditz [:dveditz] 2008-04-01 23:48:15 PDT
Nominating to consider for the inserted 1.8.1.14 regression-fix release
Comment 24 Daniel Veditz [:dveditz] 2008-04-03 13:06:50 PDT
Since this can break extension compatibility, and we were willing to put it on the "will take if we need another RC" list for 1.8.1.13, this should go in the 1.8.1.14 firedrill release.
Comment 25 Daniel Veditz [:dveditz] 2008-04-03 13:22:30 PDT
Comment on attachment 309196 [details] [diff] [review]
Don't set referrer for code from the system principal - 1.8.1 branch version

approved for 1.8.1.14, a=dveditz for release-drivers.

For the 1.8.1.14 release please land on the GECKO181_20080311_RELBRANCH
Comment 26 Daniel Veditz [:dveditz] 2008-04-03 14:21:29 PDT
Checked into GECKO181_20080311_RELBRANCH

Checking in content/base/src/nsXMLHttpRequest.cpp;
/cvsroot/mozilla/content/base/src/nsXMLHttpRequest.cpp,v  <-- 
nsXMLHttpRequest.cpp
new revision: 1.156.2.12.4.1; previous revision: 1.156.2.12
done
Comment 27 Al Billings [:abillings] 2008-04-07 15:56:43 PDT
Is there an easy way to verify this fix in the 1.8.1.14 build?

Note You need to log in before you can comment on or make changes to this bug.