1315195 - Update LoadingPrincipal as well as PrincipalToInherit to be prefixed with 'Get' since both can return null within nsILoadInfo

Status: RESOLVED DUPLICATE of bug 1627707

(Core :: DOM: Security, defect, P2)

Comment 1

[Khyati Agarwal]

Hi, I want to work on this. Could you guide me a little?

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Khyati Agarwal from comment #1)

Hi, I want to work on this. Could you guide me a little?

Sure thing - within the *.idl [1] you have to update the line to be prefixed with 'Get', something like:

[noscript, notxpcom, nostdcall, binaryname(GetLoadingPrincipal)]
nsIPrincipal binaryGetLoadingPrincipal();

and then also update that in the *.cpp [2] to ::GetLoadingPrincipal() and ultimately update all callsites (the compiler will tell you :-)). Ultimately do the same thing for PrincipalToInherit - same idea!

Thanks for taking on the work!

[1] https://searchfox.org/mozilla-central/source/netwerk/base/nsILoadInfo.idl#268
[2] https://searchfox.org/mozilla-central/source/netwerk/base/LoadInfo.cpp#616

Comment 3

[Khyati Agarwal]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #2)

Sure thing - within the *.idl [1] you have to update the line to be prefixed with 'Get', something like:

[noscript, notxpcom, nostdcall, binaryname(GetLoadingPrincipal)]
nsIPrincipal binaryGetLoadingPrincipal();

So every occurrence of LoadingPrinciple need to be replaced with GetLoadingPrinciple, e.g., aLoadingPrinciple, mLoadingPrinciple, sandboxedLoadingPrinciple, etc. Am I correct?

Also, there is also a 'loadingPrinciple'. Do I have to change anything with that also?

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Khyati Agarwal from comment #3)

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #2)

Sure thing - within the *.idl [1] you have to update the line to be prefixed with 'Get', something like:

[noscript, notxpcom, nostdcall, binaryname(GetLoadingPrincipal)]
nsIPrincipal binaryGetLoadingPrincipal();

So every occurrence of LoadingPrinciple need to be replaced with GetLoadingPrinciple, e.g., aLoadingPrinciple, mLoadingPrinciple, sandboxedLoadingPrinciple, etc. Am I correct?

Not quite - the only changes you have to perform within LoadInfo.cpp are actually replacing
::LoadingPrincipal() with ::GetLoadingPricnipal().

With callsites I mean, every caller that does
loadinfo->LoadingPrincipal() now has to be updated to loadinfo->GetLoadingPrincipal(), most of them you will catch using that query:
https://searchfox.org/mozilla-central/search?q=loadInfo-%3ELoadingPrincipal(&case=false&regexp=false&path=

Comment 5

[Khyati Agarwal]

Attachment: bug 1315195 - Update LoadingPrincipal as well as PrincipalToInherit to be prefixed with 'Get' since both can return null within nsILoadInfo

Update LoadingPrincipal as well as PrincipalToInherit to be prefixed with 'Get' since both can return null within nsILoadInfo

Comment 6

[Khyati Agarwal]

Please check the patch submitted.

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Khyati Agarwal from comment #6)

Please check the patch submitted.

Thanks - I accepted the revision in phab!

Comment 8

[Khyati Agarwal]

Thanks for accepting.
Could you suggest some other bugs like this?

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Khyati Agarwal from comment #8)

Thanks for accepting.
Could you suggest some other bugs like this?

Let me look through the backlog and reply to your email.

Comment 10

[Noemi Erli[:noemi_erli]]

Unable to land this bug:
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. (255, 'applying /tmp/tmp5ZLpB6\npatching file dom/file/uri/BlobURLProtocolHandler.cpp\nHunk #1 FAILED at 174\n1 out of 2 hunks FAILED -- saving rejects to file dom/file/uri/BlobURLProtocolHandler.cpp.rej\nabort: patch failed to apply', '')

Comment 11

[Khyati Agarwal]

When I'm trying to build it again, I'm getting this error:

Exception: Unexpected overloaded virtual method GetLoadingPrincipal in interface nsILoadInfo.
What do you suggest?

Comment 12

[Khyati Agarwal]

When I'm trying to build it again, I'm getting this error:

Exception: Unexpected overloaded virtual method GetLoadingPrincipal in interface nsILoadInfo.

What do you suggest?

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

Baku, we wanted to update the C++ friendly version of loadingPrincipal to be called ::GetLoadingPrincipal() instead of ::LoadingPrincipal() indicting the getter can return null. It seems we do not allow overloaded virtual methods (I wonder why that build before).

Anyway, I suppose we could also add a different binaryName for the JS implementations, right?

More precisely if we do:

• [binaryname(GetLoadingPrincipalFromScript)]
  readonly attribute nsIPrincipal loadingPrincipal;

  /**

  • A C++-friendly version of loadingPrincipal.
    */

• [noscript, notxpcom, nostdcall, binaryname(LoadingPrincipal)]
• nsIPrincipal binaryLoadingPrincipal();

• [noscript, notxpcom, nostdcall, binaryname(GetLoadingPrincipal)]
• nsIPrincipal binaryGetLoadingPrincipal();

The I guess we have to update the implementation of that to:

• LoadInfo::GetLoadingPrincipal(nsIPrincipal** aLoadingPrincipal) {

• LoadInfo::GetLoadingPrincipalFromScript(nsIPrincipal** aLoadingPrincipal) {

and also update:

• nsIPrincipal* LoadInfo::LoadingPrincipal()

• nsIPrincipal* LoadInfo::GetLoadingPrincipal()

but everything else in JS code should remain untouched?

I suppose that sounds like a reasonable path forward, or am I missing something?

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #13)

I don't know why my markup got destroyed so terribly. In short:

[binaryname(GetLoadingPrincipalFromScript)]
readonly attribute nsIPrincipal loadingPrincipal;

[noscript, notxpcom, nostdcall, binaryname(GetLoadingPrincipal)]
nsIPrincipal binaryGetLoadingPrincipal();

This is how the new *.dil would look like. I hope you can decipher what I meant above.

Comment 15

[Andrea Marchesini [:baku]]

[noscript, notxpcom, nostdcall, binaryname(GetLoadingPrincipal)]
nsIPrincipal binaryGetLoadingPrincipal();

This makes sense. What about: 'nativeLoadingPrincipal()' without 'Get'? Can the method return null?

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

Hey Khyati, you already have done all (at least most) of the work for this bug - any chance you would like to finish it up and get it landed?

Comment 17

[Khyati Agarwal]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #16)

Hey Khyati, you already have done all (at least most) of the work for this bug - any chance you would like to finish it up and get it landed?

Hi, sorry for the late reply, I was busy with exams. I have made the changes you mentioned, but I'm getting few errors like:

---

0:11.16 In file included from /home/khyati/src/mozilla-unified/obj-x86_64-pc-linux-gnu/netwerk/base/Unified_cpp_netwerk_base0.cpp:92:
0:11.16 /home/khyati/src/mozilla-unified/netwerk/base/LoadInfo.cpp:611:11: error: out-of-line definition of 'GetLoadingPrincipalFromScript' does not match any declaration in 'mozilla::net::LoadInfo'; did you mean 'GetGetLoadingPrincipalFromScript'?
0:11.16 LoadInfo::GetLoadingPrincipalFromScript(nsIPrincipal** aLoadingPrincipal) {
0:11.16 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
0:11.16 GetGetLoadingPrincipalFromScript
0:11.16 /home/khyati/src/mozilla-unified/netwerk/base/LoadInfo.h:54:3: note: 'GetGetLoadingPrincipalFromScript' declared here
0:11.16 NS_DECL_NSILOADINFO
0:11.16 ^
0:11.17 /home/khyati/src/mozilla-unified/obj-x86_64-pc-linux-gnu/dist/include/nsILoadInfo.h:748:14: note: expanded from macro 'NS_DECL_NSILOADINFO'
0:11.17 NS_IMETHOD GetGetLoadingPrincipalFromScript(nsIPrincipal aLoadingPrincipal) override;
0:11.17 ^
0:11.25 In file included from /home/khyati/src/mozilla-unified/obj-x86_64-pc-linux-gnu/netwerk/base/Unified_cpp_netwerk_base0.cpp:92:
0:11.25 /home/khyati/src/mozilla-unified/netwerk/base/LoadInfo.cpp:627:11: error: out-of-line definition of 'GetPrincipalToInheritFromScript' does not match any declaration in 'mozilla::net::LoadInfo'
0:11.25 LoadInfo::GetPrincipalToInheritFromScript(nsIPrincipal aPrincipalToInherit) {
0:11.25 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
0:11.37 /home/khyati/src/mozilla-unified/netwerk/base/LoadInfo.cpp:633:11: error: out-of-line definition of 'SetPrincipalToInherit' does not match any declaration in 'mozilla::net::LoadInfo'
0:11.37 LoadInfo::SetPrincipalToInherit(nsIPrincipal* aPrincipalToInherit) {
0:11.37 ^~~~~~~~~~~~~~~~~~~~~
0:11.37 /home/khyati/src/mozilla-unified/netwerk/base/LoadInfo.cpp:652:16: error: no member named 'GetPrincipalToInheritFromScript' in 'mozilla::BasePrincipal'
0:11.37 return prin->GetPrincipalToInheritFromScript(uri);
0:11.37 ~~~~ ^
0:13.19 4 errors generated.

---

Comment 18

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:ckerschb, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 19

[Christoph Kerschbaumer [:ckerschb]]

This has been fixed within Bug 1627707.