1454588 - Mark XMLHttpRequest's mozAnon and mozSystem ChromeOnly

Status: NEW

(Core :: DOM: Networking, enhancement, P3)

Description

[Anne (:annevk)]

It'd be ideal if XMLHttpRequest's constructor argument was also only in effect for chrome code and no coercion took place for web content.

Comment 1

[Thomas Wisniewski [:twisniewski]]

bz, is there even a point to having a mozSystem flag if it will be [ChromeOnly]? From what I can tell it would always be true for chrome callers, since:

>bool
>XMLHttpRequestMainThread::MozSystem() const
>{   
>  return IsSystemXHR();
>} 
> 
>XMLHttpRequestMainThread::IsSystemXHR() const
>{   
>  return mIsSystem || nsContentUtils::IsSystemPrincipal(mPrincipal);
>}

So even if callers set it to false, it's still going to be true.

Comment 2

[Thomas Wisniewski [:twisniewski]]

In the end I just took a stab at this in the following patch: https://reviewboard.mozilla.org/r/261444/diff/1#index_header (a try run seems: fine https://treeherder.mozilla.org/#/jobs?repo=try&revision=96b89aa32037089e7352d02b245cce983dd07c8d)

I found that I could remove mozSystem and the moz-specific constructor and its parameter object just fine, leaving just mozAnon as a ChromeOnly property. This of course required moving some plain mochitests over to chrome ones, but overall it was a more straightforward effort than I expected.

I'm just not sure how I should split this patch up. Should I put each test-change in its own patch to be reviewed, with the core non-test changes to the webidl, dom/xhr, and dom/workers in their own patch? What do you think, bz?

Comment 3

[Boris Zbarsky [:bzbarsky]]

OK.  So on mainthread, it looks like the XHR constructor is the only way to create an XHR, and it sets the XHR's mPrincipal to the principal of the global the constructor came from.  This is currently only allowed if that principal has the systemXHR permission.  Apart from tests and the system principal, does anything else have that permission?

If not, I agree that modulo said tests just having IsSystemXHR() return IsSystemPrincipal(mPrincipal) is fine for mainthread.

That said, the mozSystem parameter also serves to force mIsAnon to true, and this part is _not_ automatic for the system principal.  What is the plan for this aspect of the behavior?

For workers, looks like the XHR params are allowed only if the worker is loaded in a window scope and that has the "systemXHR" permission or being loaded in a non-window scope (which asserts chrome worker).  Worker XHRs do _not_ seem to default to the "mMozSystem == true" behavior if the boolean is not explicitly set in the constructor, right?  That said, since this code proxies to the mainthread XHR it might not matter in practice anyway; apart from the mozSystem getter, all the other behavior for system-principal things is as if mozSystem were set, right?  Again, except for the handling of mMozAnon.

As far as patch organization, I think it would be fine to have one patch for all the tests to stop using the API (presumably including making mozAnon writable) and a second patch to remove the C++ guts.

Comment 4

[Thomas Wisniewski [:twisniewski]]

>Apart from tests and the system principal, does anything else have that permission?

Not that I can see. Based on a grep of the central tree, that permission is only referenced by tests and in the two places where its value is checked (WorkerPrivate::GetLoadInfo and XMLHttpRequestMainThread::InitParameters). We might even be able to simply remove it, since the tests requesting it either won't need it anymore (being chrome tests now) or are passing even without it.


>That said, the mozSystem parameter also serves to force mIsAnon to true

My current plan was to simply to update any spots in the codebase that passed mozSystem:true to the constructor. Now they just call the plain constructor, and then immediately set mozAnon=true on the next line, just in case. Would that be sufficient? I suppose we could also consider adding linting rules to detect attempts to continue using mozSystem.


>one patch for all the tests to stop using the API and a second patch to remove the C++ guts

Alright, makes sense.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> Now they just call the plain constructor, and then immediately set mozAnon=true on the next line

That would work.

Comment 6

[Thomas Wisniewski [:twisniewski]]

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

Comment 7

[Thomas Wisniewski [:twisniewski]]

Attachment: Bug 1454588 - Remove moz-specific XHR constructor and XHR mozSystem property, and make mozAnon property ChromeOnly. r?bzbarsky

Remove moz-specific XHR constructor and XHR mozSystem property, and make mozAnon property ChromeOnly

Comment 8

[Thomas Wisniewski [:twisniewski]]

Oops, did you want me to still split this patch up so the test-changes are in one patch, and the C++ changes in another, bz?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Nah, it's fine.

Comment 10

[Thomas Wisniewski [:twisniewski]]

baku, I'm not sure if you caught the discussion on the patch review here, but I could use some insight/help here.

Comment 11

[Andrea Marchesini [:baku]]

I wrote a comment. It seems that we should use the new mozAnon/mozSystem attributes. Maybe you need to wrap the object using SpecialPower. Let me know if there is anything else blocking you.

Comment 12

[Thomas Wisniewski [:twisniewski]]

Weird, I still don't see a comment on the phab revision. I'm also getting rid of mozSystem in this patch, and only keeping mozAnon (not that I think that matters). But I'll see what I can figure out with SpecialPower when I have time.