1517483 - Get rid of nsIScriptSecurityManager::IsSystemPrincipal

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P3)

Description

[Boris Zbarsky [:bzbarsky]]

This state can be queried from the principal directly, in both JS and C++.

Comment 1

[Rohan]

Hi, I am new to open source and looking for my first bug. can I start with this bug, looking forward to getting your guide in order to understand the problem.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Hello, Rohan.

The idea here is to find places where nsIScriptSecurityManager::IsSystemPrincipal is called and replace those with IsSystemPrincipal checks on the principal itself.

For C++ callers, you want to look at https://searchfox.org/mozilla-central/search?q=symbol:_ZN24nsIScriptSecurityManager17IsSystemPrincipalEP12nsIPrincipal&redirect=false and https://searchfox.org/mozilla-central/search?q=symbol:_ZN24nsIScriptSecurityManager17IsSystemPrincipalEP12nsIPrincipalPb&redirect=false

For JS callers you want to look at https://searchfox.org/mozilla-central/search?q=symbol:%23isSystemPrincipal%2C_ZN24nsIScriptSecurityManager17IsSystemPrincipalEP12nsIPrincipalPb&redirect=false (which may also include some C++ callers).

I would probably recommend creating three separate changesets here: one to remove C++ callers, one to remove JS callers, one to remove the API completely.

Please feel free to ask around in the #introduction channel on irc.mozilla.org for help getting a build environment set up if you haven't done already, or feel free to e-mail me directly for help with that.

Comment 3

[Sakshaat Choyikandi]

Hi Boris, if Rohan isn't still working on it, is it ok if I give this bug a shot? I have built the codebase and found the affected files, I think I know what needs to be changed, and I'll try to split it between 3 change sets.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Sakshaat, feel free to work on this! Let me know if you need anything.

Comment 5

[Sakshaat Choyikandi]

Hi Boris, I have fixed replaced a few of the uses, however, I am a little stuck with this particular case:

https://searchfox.org/mozilla-central/source/dom/security/FramingChecker.cpp#100

I am wondering how I should tackle this because it seems like it calls the function with a pointer and the function sets a truth value to the function, however, it also returns an nsresult type. How should I proceed with this? My intuition was to convert the following invocation

if (NS_SUCCEEDED(
        ssm->IsSystemPrincipal(topDoc->NodePrincipal(), &system)) &&
    system) {
    // Found a system-principled doc: last docshell was top.
    break;
}

into

if (NS_SUCCEEDED(
        topDoc->NodePrincipal())->GetIsSystemPrincipal(&system)) &&
    system) {
    // Found a system-principled doc: last docshell was top.
    break;
}

Am I on the right track? The code seemed to build just fine, however, that doesn't necessarily imply that it is correct. I would appreciate any help with this. I was also hoping you could help me figure out how I could test if I am breaking anything. I tried to just run ./mach gtest but I am unsure if that's the right set of tests. Thanks!

Comment 6

[Boris Zbarsky [:bzbarsky]]

There are two relevant methods on nsIPrincipal.

There's nsIPrincipal::GetIsSystemPrincipal(), which returns an nsresult and takes a bool* argument, which it uses to set a boolean to a value. This method is basically there only for historical reasons and you shouldn't really use it. The nsresult it returns is always NS_OK, for what it's worth.

There's also nsIPrincipal::IsSystemPrincipal() which takes no arguments and returns a boolean. That's what you should use.

So the code in FramingChecker.cpp should become:

  if (topDoc->NodePrincipal()->IsSystemPrincipal()) {
    break;
  }

and the system local can go away entirely.

The code with your change would work (if the stray ')' after NodePrincipal() is removed); it's the right way to use nsIPrincipal::GetIsSystemPrincipal(). But it's not ideal. ;)

As far as tests, you have a few options. You could try running them locally, but the full set of test suites takes quite a while to run (and for a lot of them using the computer while they're running is not simple). The better options are to either attach the patches to this bug (or submit them to Phabricator) and have me push them to try for you or to follow the directions at https://wiki.mozilla.org/ReleaseEngineering/TryServer#Getting_access_to_the_Try_Server to get access to the try server yourself.

Comment 7

[Sakshaat Choyikandi]

Attachment: Bug 1517483 - Get rid of nsIScriptSecurityManager::IsSystemPrincipal r=bzbarsky

Replaced instances of callers in both C++ and JS files to query the state from the principal directly.

Comment 8

[Sakshaat Choyikandi]

Hi Boris, I've created a patch for the issue. After I had committed I realized that you said you recommend two different change sets for the C++ and JS callers, so I am very sorry about that. I still have to remove the calls but I was just wondering if there any issues so far. Thanks for your help!

Comment 9

[Boris Zbarsky [:bzbarsky]]

It's not a big deal to have the one changeset. I commented on the Phabricator revision; the main issue at the moment is that it's against a slightly old tree, so doesn't apply on tip.

Comment 10

[Sakshaat Choyikandi]

Hi Boris, that is very odd. I have pulled from the mozilla-central repository by running hg pull and I also ran hg update, but for some reason it the file that you mentioned that shouldn't exist on TIP still exists. Am I missing something? I am a git user and don't know if I am using mercurial and arcanist right. Thanks!

Comment 11

[Boris Zbarsky [:bzbarsky]]

Hmm. "hg update" run with no args will do something that depends on your exact branch setup.

What does "hg heads" show for you in that repository right now? What about "hg log -r default"? What about "hg parent"?

Comment 12

[Sakshaat Choyikandi]

I was having really weird issues with mercurial where supposedly my entire repository was corrupted, I just set up a new environment by cloning a fresh version of mozilla-central and applying the diff file from Phabricator. It seems to have worked because the PeerConnection.js file does not exist in this version. I am currently working on fixing the issues you mentioned and then removing the API entirely, I'll submit a patch when it is done. Thank you so much for your help so far!

Comment 13

[Sakshaat Choyikandi]

Attachment: Bug 1517483 - Fixed issues from the last patch (D22532) and also removed definitions of the API. r=bzbarsky

Getting rid of nsIScriptSecurityManager::IsSystemPrincipal by changing all usages to query from the principal itself, and removing the API

Comment 14

[Boris Zbarsky [:bzbarsky]]

Hey, just to check, are you still working on this or planning to? If you're stuck on something, please let me know and I'll see what I can do to help!

Comment 15

[Boris Zbarsky [:bzbarsky]]

Note: some of the changes being made in https://phabricator.services.mozilla.com/D23038 will conflict with these changes and need a merge.

Comment 16

[Sakshaat Choyikandi]

Hi Boris, sorry I got really busy with some school work but I can work on it again now. So I looked at the tests, I have fixed the issues that were relatively easy to find such as removing unused vars, etc. I am still trying to figure out how I can find which files may have caused the error for some of the other failures, so I can begin trying to fix them. Right now, for some of the failures, it's not clear to me what may have caused it and where I should look to fix the issue. I would really appreciate any help with that.

For now, I am working on finding more instances of the command being used in places that I may have missed. Those files surprisingly did not show up when I used the links you provided in this thread. I tried to use searchfox to find instances that I may have missed, but I couldn't figure out how to really query it, so I am using grep to essentially find a replace them.

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #15)

Note: some of the changes being made in https://phabricator.services.mozilla.com/D23038 will conflict with these changes and need a merge.

Should I wait for this to be merged to mozilla-central or is there something else that I need to do beforehand?

Thanks for all of your help!

Comment 17

[Boris Zbarsky [:bzbarsky]]

Right now, for some of the failures, it's not clear to me what may have caused it and where I should look to fix the issue. I would really appreciate any help with that.

OK. Which failures are being troublesome in particular? Do you know how to run the relevant tests yourself?

Those files surprisingly did not show up when I used the links you provided in this thread

My apologies. Apparently the "search for IDL method" bit in searchfox searches .js and .xml files but not .html ones. :(

I don't think searchfox will do anything grep doesn't in this case, since it's not pre-indexing this stuff in a useful way.

Should I wait for this to be merged to mozilla-central or is there something else that I need to do beforehand?

It's just something to watch out for. If your changes are ready before that merges, it's fine to go ahead and land and bug 1508817 will get merged on top. If not, then you will need to merge on top of those changes.

Comment 18

[Sakshaat Choyikandi]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #17)

OK. Which failures are being troublesome in particular? Do you know how to run the relevant tests yourself?

For example, the Gtest failure under Linux x64 asan, I see that it has failed but I'm not sure where to find out what test case caused the error. I'm not entirely sure how to run the tests, is it just ./mach test gtest?

It's just something to watch out for. If your changes are ready before that merges, it's fine to go ahead and land and bug 1508817 will get merged on top. If not, then you will need to merge on top of those changes.

Got it, thanks!

Comment 19

[Boris Zbarsky [:bzbarsky]]

but I'm not sure where to find out what test case caused the error

The log line for that failure is:

TEST-UNEXPECTED-FAIL | WebRtcIceConnectTest.TestNetworkOnlineTriggersConsent | Value of: res

Searchfox says "TestNetworkOnlineTriggersConsent" is https://searchfox.org/mozilla-central/rev/358f816f63da072145c593e9e2ac36b7250ecd25/media/mtransport/test/ice_unittest.cpp#3240-3253 which is of somewhat limited usefulness in terms of figuring out what's going wrong.

You can run that specific test using "mach gtest WebRtcIceConnectTest.TestNetworkOnlineTriggersConsent". Running all of gtest is possible with "mach gtest", but would take a long time and be annoying. ;)

What I expect is going on with that specific test is that it's calling into PeerConnection.jsm and running into https://searchfox.org/mozilla-central/rev/358f816f63da072145c593e9e2ac36b7250ecd25/dom/media/PeerConnection.jsm#412 not having been updated.

Comment 20

[Sakshaat Choyikandi]

That's very helpful, thank you.

Comment 21

[Boris Zbarsky [:bzbarsky]]

So yeah, autolanding https://phabricator.services.mozilla.com/D22532 failed because bug 1508817 had already landed. Do you want to do the merging on top of that, or should I just deal with it?

Comment 22

[Sakshaat Choyikandi]

I can do it. Does it just involve pulling, fixing a conflict, and re-uploading another differential?

Comment 23

[Boris Zbarsky [:bzbarsky]]

Well, updating the existing differential revision. But yes, that's all that's needed.

Comment 24

[Sakshaat Choyikandi]

Right, got it.

Comment 25

[Sakshaat Choyikandi]

I've updated the differential, does it look okay? I had a bit of trouble trying to figure out how to use vimdiff, so I just wanted to make sure.

Comment 26

[Boris Zbarsky [:bzbarsky]]

I commented on the differential revision.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/16bf818b02ac
Get rid of nsIScriptSecurityManager::IsSystemPrincipal r=bzbarsky

Comment 28

[Boris Zbarsky [:bzbarsky]]

Jorg, there's one user in SeaMonkey at https://searchfox.org/comm-central/rev/b861a92a529f2820db0078a3ace7643f3c80ba59/suite/components/bindings/preferences.xml#153

Is that code still maintained?

Comment 29

[Jorg K (CEST = GMT+2)]

Good question. I'm not maintaining it. Let's ask FRG.

Comment 30

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/16bf818b02ac

Comment 31

[Frank-Rainer Grahl (:frg)]

Jorg, there's one user in SeaMonkey at...
Good question. I'm not maintaining it. Let's ask FRG.
Thanks Jorg. will see that it gets fixed when we tackle a future release after 2.57.

Comment 32

[Boris Zbarsky [:bzbarsky]]

Sakshaat, thank you very much for fixing this! I thought I'd said that already, but I apparently failed to submit in that window.

Comment 33

[Sakshaat Choyikandi]

I think I learned a lot from this bug fix. Thank you for all of your help!

Comment 34

[Boris Zbarsky [:bzbarsky]]

You're very welcome.