1228139 - Remove nsIURIWithPrincipal

Status: RESOLVED FIXED

(Core :: Networking, defect, P3)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

We should remove nsIURIWithPrincipal. It was added to enable us to derive the origin of blob: URIs, however these days we encode the origin in the blob: URI and so it is no longer needed.

We also need to do this in order to fix bug 1228115 since nsIPrincipal's aren't threadsafe and so prevents instantiating nsIURIWithPrincipals off the main thread.

Comment 1

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b7d70c463ba

Comment 2

[Dragana Damjanovic [:dragana]]

Cleaning up necko-backlog, I tried to make  patch for this bug. Simple removing nsIURIWithPrincipal does not work.
I need more info here, or there is another bug.

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yeah, this isn't as simple as just removing the interface. We still use nsIURIWithPrincipal for blob: URIs and depend on the functionality that it provides, so we need to replace the use of nsIURIWithPrincipal with something else.

These days a blob: URL looks like "blob:https://bugzilla.mozilla.org/e1deebac-4cab-8147-b3c4-62692f716201".

Note that we currently don't parse this as a "nested" URL, i.e. they currently do not implement nsINestedURI. The only thing we do currently for blob: URLs is that we make them implement nsIURIWithPrincipal, and then we stick an nsIPrincipal in them. In the above case we use a codebase principal with URI "https://bugzilla.mozilla.org".


So in order to remove nsIURIWithPrincipal, we need to check which places it is used, and replace those places with something else.

There aren't too many places where nsIURIWithPrincipal is used.
http://mxr.mozilla.org/mozilla-central/search?string=nsiuriwithprincipal

It looks to me like all of those places can be fixed by making these URIs actually parse as "nested" URLs. And the remaining ones would be fixed by making sure we actually step into nested URIs as we should.

Comment 4

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 5

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 6

[Andrea Marchesini [:baku]]

Attachment: first experiment

This is just an experiment. So far it looks promising on try but I need a feedback.

The idea here is:

1. BlobURLs are treated similar to simple URLs. It's impossible to have nsIPrincipal of the creator from them.

2. When we need to obtain a nsIPrincipal from a blobURL, we consider the 'path' part of it, because that is the origin of the 'creator'. (Note that a blobURL has this format: blob:<origin>/<uuid>). If the 'creator' runs with SystemPrincipal or a NullPrincipal, the path will be 'null' (as string). In this case, a NullPrincipalURI() will be used.

3. similar approach is done when we want to check if a nsIPrincipal can load a blobURL.

4. when we need to retrieve the nsIPrincipal from a BlobURLChannel, let's use a nsIBlobURLChannel instead of nsILoadInfo or the final URL.

5. BlobURLChannel checks if the loadingPrincipal subsumes the blobURL nsIPrincipal when necko asks for an nsIInputStream.

In the end, I'm moving the security check at a nsIChannel level. This will be useful when I'll work on removing the broadcasting of blobURLs to any process and BlobURLChannel will be able to retrieve the inputStream of the blob from the parent process.This is just an experiment. So far it looks promising on try but I need a feedback.

The idea here is:

1. BlobURLs are treated similar to simple URLs. It's impossible to have nsIPrincipal of the creator from them.

2. When we need to obtain a nsIPrincipal from a blobURL, we consider the 'path' part of it, because that is the origin of the 'creator'. (Note that a blobURL has this format: blob:<origin>/<uuid>). If the 'creator' runs with SystemPrincipal or a NullPrincipal, the path will be 'null' (as string). In this case, a NullPrincipalURI() will be used.

3. similar approach is done when we want to check if a nsIPrincipal can load a blobURL.

4. when we need to retrieve the nsIPrincipal from a BlobURLChannel, let's use a nsIBlobURLChannel instead of nsILoadInfo or the final URL.

5. BlobURLChannel checks if the loadingPrincipal subsumes the blobURL nsIPrincipal when necko asks for an nsIInputStream.

In the end, I'm moving the security check at a nsIChannel level. This will be useful when I'll work on removing the broadcasting of blobURLs to any process and BlobURLChannel will be able to retrieve the inputStream of the blob from the parent process.

Comment 7

[Boris Zbarsky [:bzbarsky]]

I'm not quite sure I understand how this is working in the nullprincipal cases.

If the creator has a nullprincipal, the blob url will have a different nullprincipal, right?  What does that mean for the "loadingPrincipal subsumes the blobURL nsIPrincipal" check, exactly?

If I have a sandboxed page that loads an image from blob and paints it into a canvas, will that taint the canvas with the proposed changes?  Why or why not?  (Does it taint the canvas per spec, btw?)

Comment 8

[Andrea Marchesini [:baku]]

Attachment: remove nsIURIWithPrincipal

I want to split the work in multiple patches and multiple bugs. This patch is just about removing nsIURIWithPrincipal. The broadcasting of BlobURL is still a thing but it will be removed in a follow up.

Comment 9

[Andrea Marchesini [:baku]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=476916ddfedd944d20aea0b3ae5e32bab184fdc8&selectedJob=185558486

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8988710 [details] [diff] [review]
remove nsIURIWithPrincipal

I'm sorry for the lag here.  I'm still catching up on vacation bits, and this needed careful reading.

>BlobURLProtocolHandler::GetPrincipalOrURIFromBlobURL() that retrieves the
>nsIPrincipal from this hashtable.

I think we should just call this GetBlobURLPrincipal and have callers that want the principal URI get it themselves as needed.  That simplifies the API significantly.  For example, at that point we could just return nsIPrincipal*, with null meaning "revoked URI" or whatnot.

>An extra complexity is about revoked BlobURLs which are kept alive for 5
>seconds in order to allow async operations to complete. For revoked blobURL,
>nsIURIWithPrincipal returns a null nsIPrincipal.

I think this would be clearer if you made it clear that for blob url objects created from a string that has been revoked nsIURIWithPrincipal returns a null principal.  That is, the things that get revoked are strings, not nsIURI objects, right?

>+++ b/caps/BasePrincipal.cpp
>+  if (mozilla::dom::BlobURLProtocolHandler::GetPrincipalOrURIFromBlobURL(aURI,
>+                                                                         getter_AddRefs(blobPrincipal),
>+                                                                         getter_AddRefs(blobURI))) {
>+    MOZ_ASSERT(blobURI || blobPrincipal);

Can't we just assert blobPrincipal?  Looks to me like any time GetPrincipalOrURIFromBlobURL returns true it will hand out a non-null principal...

>+    if (blobPrincipal) {

So this test would always be true.

>+    return CreateCodebasePrincipal(blobURI, aAttrs, aOriginNoSuffix);

And this is dead code.

Is there a reason to not keep the old behavior, where if we have a blob URL but it has no principal we fall back on a nullprincipal?  I guess in the new world we have a harder time telling apart the "not a blob url" case from the "revoked blob url" or whatnot case?

So maybe we do in fact need a return value that indicates "yeah, this was a blob url, but it had no principal"...

>+++ b/caps/ContentPrincipal.cpp
>@@ -159,25 +160,28 @@ ContentPrincipal::GenerateOriginNoSuffix
>+    MOZ_ASSERT(blobPrincipal || blobURI);

Again, blobPrincipal is guaranteed non-null here.

>+    return nsContentUtils::GetASCIIOrigin(blobURI, aOriginNoSuffix);

So this is dead code.

> ContentPrincipal::MayLoadInternal(nsIURI* aURI)
>+    MOZ_ASSERT(blobURI || blobPrincipal);

And here too...

>+    return MayLoadInternal(blobURI);

Dead code.

>+++ b/caps/NullPrincipal.cpp
> NullPrincipal::MayLoadInternal(nsIURI* aURI)
>-  // Also allow the load if we are the principal of the URI being checked.

Why is this comment being removed?

>+++ b/caps/OriginAttributes.cpp
>@@ -58,28 +58,31 @@ OriginAttributes::SetFirstPartyDomain(co
>+    MOZ_ASSERT(blobURI || blobPrincipal);

Usual deal: blobPrincipal is not null here.

>+    return SetFirstPartyDomain(aIsTopLevelDocument, blobURI);

Dead code.

>+++ b/dom/file/uri/BlobURL.cpp
>+  : mCreationTime(PR_Now())

This doesn't seem reliable if you then plan to compare it to the revocation time.  PR_Now() can run backwards in some cases, so things can end up looking not-revoked even though they are revoked.  Can we use a TimeStamp here, or do we need to compare this across processes (as opposed to just back in the process it initially came from)?

If all we care about this time for is "was the URI already revoked when we create it", can't we just record that information in BlobURLProtocolHandler::NewURI?  Or is the whole issue that you don't want to have the hashtable available there?

> BlobURL::ReadPrivate(nsIObjectInputStream *aStream)

We're changing the serialization/deserialization format of blob URLs here, right?  Have you tested what happens with session restore across this change when a blob url is open in a tab?  Do we properly restore that tab?  Or is that generally broken with session restore I guess?  Do we properly restore _other_ tabs?  Please test this!

>+  rv = aStream->Read64((uint64_t*)&mCreationTime);

I would prefer if we read into an on-stack uint64_t and then assigned to mCreationTime.  That would mean that we have symmetric casts (static cast through integer conversions) on read and write, instead of  having this reinterpret_cast on pointers on read.  In practice I expect it's all the same, but it's clearer if the conversions are symmetric.

>-  // Disallow setting the scheme, since that could cause us to be associated
>-  // with a different protocol handler that doesn't expect us to be carrying
>-  // around a principal with nsIURIWithPrincipal.

Still worth documenting why we don't want to allow changing the scheme.

>@@ -201,35 +151,26 @@ BlobURL::EqualsInternal(nsIURI* aOther,
>+  // We don't want to compare the creationTime.

So we used to consider a revoked and a non-revoked blob url not equal.  This new code will consider them equal.  Is that the behavior we want?

>+++ b/dom/file/uri/BlobURLProtocolHandler.cpp
>+#include "BlobURL.h"

Please include this from the canonical path (mozilla/dom/BlobURL.h), like this file used to.  There are some issues with the build system, last I checked, when the same file is included via multiple different paths...

>+  if (!aAlsoIfRevoked && res && res->mRevokeTime) {

It might be nice to abstract the mRevokeTime thing behind an IsRevoked() method.

>@@ -676,24 +667,26 @@ BlobURLProtocolHandler::RemoveDataEntry(
>+    BroadcastBlobURLUnregistration(nsCString(aUri));

We should change RemoveDataEntry to take a "const nsCString&" arg.  I think every caller has one already.

>@@ -849,55 +830,40 @@ BlobURLProtocolHandler::NewChannel2(nsIU
>+  if (!info || info->mObjectType != DataInfo::eBlobImpl || !info->mBlobImpl ||
>+      !info->mPrincipal) {

This seems like a behavior change.

In the old code, if the passed-in URI is already revoked as of when the nsIURI was created, we would get a null principal from GetPrincipal, bail out, and set the channel to InitFailed().

In the new code, the equivalent would be a creation time check or whatever test we end up with for "created already revoked", but we're not doing that here.  I assume we should be, right?

>+++ b/dom/file/uri/BlobURLProtocolHandler.h
>+  GetPrincipalOrURIFromBlobURL(nsIURI* aURI,
>+                               nsIPrincipal** aPrincipal,
>+                               nsIURI** aResult);

This needs to be documented pretty extensively, in terms of what it's returning, interactions with revocation, etc.

>+++ b/netwerk/base/nsNetUtil.cpp
>+    nsCOMPtr<nsIURI> sourceBlobURI;

Right, so this is the one place you need a URI.  You could just have a static helper here that gets the principal and then  if non-null gets the URI from it, right?

>+    if (BlobURLProtocolHandler::GetPrincipalOrURIFromBlobURL(sourceBaseURI,
>+                                                             nullptr,
>+                                                             getter_AddRefs(sourceBlobURI)) &&
>+        sourceBlobURI) {
>+        sourceBaseURI = sourceBlobURI;

This is a behavior change.  The old code would set sourceBaseURI to null for a revoked blob URI, so it would never test same-origin with anything.  Why do we want this change?

Similar for the target.

Comment 11

[Andrea Marchesini [:baku]]

> >BlobURLProtocolHandler::GetPrincipalOrURIFromBlobURL() that retrieves the
> >nsIPrincipal from this hashtable.

This is actually the first of a set of patches to get rid of the broadcasting or BlobURLs. See bug 1472158. I introduced this method because in the following patches I could not have the principal if the blobURL is owned by another process. When this happens I take the URI from the blob:<origin>/part and I return a nullptr principal. But I guess I can do this step in bug 1472158.

> Is there a reason to not keep the old behavior, where if we have a blob URL
> but it has no principal we fall back on a nullprincipal?  I guess in the new

Related to this single patch, the answer would be: no reasons. But I started working on this bug because of bug 1472158 where the answer will become: yes! we have an important reason to do not fall back on a nullprincipal: a blob URL can be owned by another process. But I can work on this in a separate bug.

> This doesn't seem reliable if you then plan to compare it to the revocation
> time.  PR_Now() can run backwards in some cases, so things can end up
> looking not-revoked even though they are revoked.  Can we use a TimeStamp
> here, or do we need to compare this across processes (as opposed to just
> back in the process it initially came from)?

We do not compare this across processes yet. But it will be done in the following bug. This is why I use PR_Now(). Plus, I don't need a fine time check: the acceptable range is 5 seconds.

> If all we care about this time for is "was the URI already revoked when we
> create it", can't we just record that information in
> BlobURLProtocolHandler::NewURI?  Or is the whole issue that you don't want
> to have the hashtable available there?

For this first step, everything is there so, a boolean would be enough. I could introduce the PRTime in bug 1472158, but I prefer to do this step here.

> We're changing the serialization/deserialization format of blob URLs here,
> right?  Have you tested what happens with session restore across this change
> when a blob url is open in a tab?  Do we properly restore that tab?  Or is
> that generally broken with session restore I guess?  Do we properly restore
> _other_ tabs?  Please test this!

SessionRestore of blob URL doesn't work because we don't keep the underlying blobImpl.
SessionStore + blobURL is already broken.

> So we used to consider a revoked and a non-revoked blob url not equal.  This
> new code will consider them equal.  Is that the behavior we want?

Yes, it is: how is it possible that the uriA.spec == uriB.spec but one has been revoked and the other is not?
It can happen only if the same UUID is generated for 2 different blobURLs.

> >+  if (!aAlsoIfRevoked && res && res->mRevokeTime) {
> 
> It might be nice to abstract the mRevokeTime thing behind an IsRevoked()
> method.

I don't do this for any other member. See mObjectType/mBlobImpl/mMediaSource/...

> We should change RemoveDataEntry to take a "const nsCString&" arg.  I think
> every caller has one already.

It's not like that in URLMainThread. I have to change that... right?

I'll submit a new patch with all your comments.

Comment 12

[Boris Zbarsky [:bzbarsky]]

> When this happens I take the URI from the blob:<origin>/part and I return a nullptr principal

I see.

This is why there should be documentation in the header for what one should expect from the function, instead of people having to guess at the API contract based on its implementation.  ;)

I think it would be better to just get rid of nsIURIWithPrincipal, without other behavior changes, in this bug, then do the URI bits in bug 1472158 or another bug sitting between them...

> This is why I use PR_Now(). Plus, I don't need a fine time check: the acceptable range is 5 seconds.

Just so we're clear, PR_Now() can go backwards by arbitrary amounts of time (think hours, months) all of a sudden.

Is the need here to compare two times initially recorded in process A but then shipped to process B and we're comparing in B?  Or is it to compare a time recorded in process A with a time recorded in process B?  I'd really like to figure out whether we can build this check on TimeStamp somehow instead of using PR_Now().

> Yes, it is: how is it possible that the uriA.spec == uriB.spec but one has been revoked and the other is not?

When one was created before the revocation, no?  I guess it depends on what "revoked" means, exactly...

> I don't do this for any other member

Sure, but here we have a member that is a non-boolean but often we want to extract a boolean from it.  I think having a getter for that boolean that encapsulates the correspondence would be a lot clearer than comparing to magic values (like 0) in tons of different places.

> It's not like that in URLMainThread. I have to change that... right?

Why?  The callers in URLMainThread are passing a nsAutoCString and a NS_LossyConvertUTF16toASCII.  NS_LossyConvertUTF16toASCII is a subclass of nsAutoCString.  nsAutoCString inherits from nsCString.

Comment 13

[Andrea Marchesini [:baku]]

Attachment: blob.patch

Comment 14

[Boris Zbarsky [:bzbarsky]]

I'm sorry for the horrible lag here.  Still trying to recover from being away for a week at end of June...

I will make sure to get to this review on Monday.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8990603 [details] [diff] [review]
blob.patch

I am really sorry about the even more lag bit.  I will commit to making review turnaround on further patches here very quick, but I would like to see the documentation and explanations I am asking for added; that will greatly aid not only reviews but further maintenance of this code.

Comparing this patch to the previous one, even the bits about revoked URLs that the previous version had in the commit message, unclear as they were, are gone now.  There is literally no documentation of any sort about what the story is for revoked urls.  Please see my comments on what I think would maybe make sense here in comment 10.  But I'm open to other things too, as long as the behavior is actually described clearly!

>+++ b/caps/BasePrincipal.cpp
>+  if (mozilla::dom::BlobURLProtocolHandler::GetBlobURLPrincipal(aURI,

No need for the "mozilla::" prefix, right?  Applies to ContentPrincipal and NullPrincipal too.

The change to make GetBlobURLPrincipal return a nullprincipal for revokes cases preserves behavior here but changes it at some of the other callsites, no?  I would really prefer it if we had an initial changeset implementing whatever behavior changes we plan to make, separate from this big rearchitecture changeset.  That way when we have compat fallout we can bisect it to the right thing, as well as clear discussion about which of the behavior changes we actually want.

>+++ b/dom/file/uri/BlobURL.cpp
> SessionRestore of blob URL doesn't work because we don't keep the underlying blobImpl.

OK, but that's only one of the questions I asked.  Do other tabs still restore properly?  If I have a page with several frames, one of which is a blob, does the page itself restore?  Do the non-blob frames restore?

Basically, do we get exceptions from the deserialization failures in sessionstore that affect anything other than the blob pages?

On the other hand, maybe we don't get exceptions.  Maybe we just end up with random-ish values of mRevoked...  The commit message should explain why the serialization changes are safe, either based on logical argument or a description of the testing performed.

>Yes, it is: how is it possible that the uriA.spec == uriB.spec but one has been revoked and the other is not?

Again, I would prefer it if the behavior change (to consider revoked and non-revoked versions of a blob url as equal) were in a separate changeset from everything else, so when we run into compat fallout we can identify the problem more easily.  This should be pretty straightforward for this particular behavior change.

>+++ b/dom/file/uri/BlobURLProtocolHandler.cpp

So there are some checks for info->mPrincipal being null here.  Can it actually be null in the current world (afaict, no)?  Is this preparing for bug 1472158 (afaict, yes)?  Documentation, please!

>+  // This method returns a nsIPrincipal from a nsIURI if that is a known
>+  // blobURL.  If the blobURL has been revoked, a NullPrincipal will be
>+  // returned.
>+  static bool
>+  GetBlobURLPrincipal(nsIURI* aURI,
>+                      nsIPrincipal** aPrincipal);

OK, what does the return value mean?  How does the return value interact with the post-call value of aPrincipal?

Please document this method in a useful way.

>+++ b/netwerk/base/nsNetUtil.cpp
>+      nsCOMPtr<nsIURI> sourceBlobURI;

This is named a bit oddly.  It's not the blob URI for the source, which is what the naming implies.  It's the URI for whoever created the source blob, right?

So maybe this should be called "sourceBlobOwnerURI"?

Similar for target.

I guess at this point we're preserving the old behavior of having revoked things not be same-origin with anything by having the URI be the nonce nullprincipal URI in those cases?

Comment 16

[Andrea Marchesini [:baku]]

Attachment: part 1 - NullPrincipal when nsIURIWithPrincipal.principal is nullptr

Comment 17

[Andrea Marchesini [:baku]]

Attachment: part 2 - BlobURL comparison

Comment 18

[Andrea Marchesini [:baku]]

> OK, but that's only one of the questions I asked.  Do other tabs still
> restore properly?  If I have a page with several frames, one of which is a
> blob, does the page itself restore?  Do the non-blob frames restore?

Yes. Other tabs/frames are restored properly. The failure of a blobURL loading, does not interfere with the loading of other tabs/iframes.

> Basically, do we get exceptions from the deserialization failures in
> sessionstore that affect anything other than the blob pages?

not in my tests:

1. I tested a blob URL opened as top-level tab via window.open() - the page was shown blank.
2. a page containing several iframes, 1 of them with a random invalid blobURL and 1 with a valid blobURL. The 2 blobURL iframes are shown blank.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8994194 [details] [diff] [review]
part 1 - NullPrincipal when nsIURIWithPrincipal.principal is nullptr

r=me.  I assume it doesn't matter that we create a new nullprincipal for each GenerateOriginNoSuffix call instead of creating one and caching it?

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8994201 [details] [diff] [review]
part 2 - BlobURL comparison

>+    MOZ_ASSERT(NS_SUCCEEDED(rv) && equal);

Why can we assert this?  We know nothing about these URIs other than that they are both blob URIs... Why should their principals be equal?

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8994194 [details] [diff] [review]
part 1 - NullPrincipal when nsIURIWithPrincipal.principal is nullptr

Oh, and the commit message here needs to explain what is actually being changed.  Right now it doesn't.  Maybe something like:

  Return a new nullprincipal when a revoked blob uri is asked for its principal.

or something?  Except that's not really what's going on here...  Really there are two separate changes that are doing somewhat different things.  Maybe they should be in separate commits with separate commit messages that actually describe them.

Comment 22

[Andrea Marchesini [:baku]]

Attachment: part 3 - removing nsIURIWithPrincipal

Added assertions, and a better documentation of what RemoveDataEntry() and GetBlobURLPrincipal() do.

Comment 23

[Andrea Marchesini [:baku]]

Attachment: part 2 - BlobURL comparison

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8994426 [details] [diff] [review]
part 2 - BlobURL comparison

r=me

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8994424 [details] [diff] [review]
part 3 - removing nsIURIWithPrincipal

>+++ b/dom/file/uri/BlobURLProtocolHandler.cpp
>+                                                         BasePrincipal::Cast(info->mPrincipal)->OriginAttributesRef())) {

Want to fix the weird over-indent while you're here?

>+++ b/dom/file/uri/BlobURLProtocolHandler.h
>+  // returns true, setting aPrincipal as the BlobURL's parent's principal

The "parent" terminology still doesn''t make sense to me.  What we really do is set aPrincipal to the principal that a channel loaded from the blob URL would have, right?

I think this would most clearly be phrased like so:

  // This method returns false if aURI is not a known BlobURL. Otherwise it
  // returns true.
  // 
  // When true is returned, the aPrincipal out param is meaningful.  It gets
  // set to the principal that a channel loaded from the blob would get if
  // the blob is not already revoked and to a NullPrincipal if the blob is
  // revoked.
  //
  // This means that for a revoked blob URL this method may either return
  // false or return true and hand out a NullPrincipal in aPrincipal,
  // depending on whether the "remove it from the hashtable" timer has
  // fired.  See RemoveDataEntry().

>+++ b/netwerk/base/nsNetUtil.cpp

So this change is safe in the revoked cases (where it used to set sourceBaseURI or targetBaseURI to null and then fail out immediately) because now we will make it down to the nsIStandardURL QIs and fail out at that point?

We should probably mention that explicitly in the commit message.

r=me.  Thank you for bearing with the multiple rounds of review here!

Comment 26

[Andrea Marchesini [:baku]]

> So this change is safe in the revoked cases (where it used to set
> sourceBaseURI or targetBaseURI to null and then fail out immediately)
> because now we will make it down to the nsIStandardURL QIs and fail out at
> that point?

It's safe because:

1. if only 1 of the 2 blobURLs is revoked, we compare the scheme of a NullPrincipalURI with a nsStandardURL.
2. if they are both nullPrincipal, we compare the 2 URLs and they will be different.

> r=me.  Thank you for bearing with the multiple rounds of review here!

Thank you.

Comment 27

[Boris Zbarsky [:bzbarsky]]

> 1. if only 1 of the 2 blobURLs is revoked, we compare the scheme of a NullPrincipalURI with a nsStandardURL.

Not necessarily.  We have  three possible states as a result of a GetBlobURLPrincipal() call:

1)  We got a useful principal.
2)  We got a nullprincipal.
3)  False got returned.

#2 and #3 both represent the "revoked" state.  In this code, #3 means that sourceBaseURI will remain the blob URI.  If both are revoked and fall into case 3, we will press on past the 

     if (!sourceBaseURI || !targetBaseURI)
         return false;

check, which we didn't use to do before.  But we still return false, and it's worth documenting why.

> 2. if they are both nullPrincipal, we compare the 2 URLs and they will be different.

Because they're not nsIStandardURL, right?

Comment 28

[Andrea Marchesini [:baku]]

> > 2. if they are both nullPrincipal, we compare the 2 URLs and they will be different.
> 
> Because they're not nsIStandardURL, right?

Exactly.

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b2a1cd818cf2
Remove nsIURIWithPrincipal - part 1 - Use of NullPrincipal when nsIURIWithPrincipal.getPrincipal() returns a nullptr, r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/3549739dde68
Remove nsIURIWithPrincipal - part 2 - BlobURL comparison should not check the principal, r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/09917998d963
Remove nsIURIWithPrincipal - part 3 - main part, r=bz

Comment 30

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/b2a1cd818cf2
https://hg.mozilla.org/mozilla-central/rev/3549739dde68
https://hg.mozilla.org/mozilla-central/rev/09917998d963