Closed Bug 1478441 Opened Last year Closed Last year

Introduce nsIURIWithSpecialOrigin for Thunderbird to replace nsIURIWithPrincipal which was removed in bug 1228139

Categories

(Core :: Networking, enhancement, P5)

enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: jorgk, Assigned: jorgk)

Details

(Whiteboard: [necko-triaged][Thunderbird-testfailure: Z all][Thunderbird-disabled-test][Thunderbird-temporary-fix])

Attachments

(4 files, 15 obsolete files)

5.50 KB, patch
Details | Diff | Splinter Review
2.66 KB, patch
Details | Diff | Splinter Review
17.35 KB, patch
jorgk
: review+
Details | Diff | Splinter Review
11.03 KB, patch
baku
: review+
Details | Diff | Splinter Review
Usage of nsIURIWithPrincipal was introduced in Mailnews in bug 1347598 when its removal was already on the books in bug 1228139 :-(

I understand that to solve the <img crossorigin="anonymous" ...> problem, we were relying on the code that just got removed here:
https://hg.mozilla.org/mozilla-central/rev/09917998d963#l1.36 or
https://hg.mozilla.org/mozilla-central/rev/09917998d963#l2.56

Boris, can you please suggest a replacement.

Meanwhile I'll have to do whatever emergency surgery that is necessary to get us compiling again.
Flags: needinfo?(bzbarsky)
Keywords: leave-open
Andrea?  Note that this is an _implementation_ of nsIURIWithPrincipal that is not blob, so I'm not sure there's a way to do this right now after bug 1228139 landed...
Flags: needinfo?(bzbarsky) → needinfo?(amarchesini)
Well, if you want to stick with the removal of nsIURIWithPrincipal, you might need some conditional code in ContentPrincipal.cpp. Something like

#ifdef MOZ_THUNDERBIRD
  nsCOMPtr<nsIMsgMailNewsUrl> uriWithPrincipal = do_QueryInterface(origin);
  if (uriWithPrincipal) {
    nsCOMPtr<nsIPrincipal> uriPrincipal;
    rv = uriWithPrincipal->GetPrincipal(getter_AddRefs(uriPrincipal));
    NS_ENSURE_SUCCESS(rv, rv);
    return uriPrincipal->GetOriginNoSuffix(aOriginNoSuffix);
  }
#endif

Intolerable, right? Let alone the problem to get to nsIMsgMailNewsUrl.
This would be the emergency surgery. I could add
  readonly attribute nsIPrincipal principal;
  readonly attribute nsIURI principalUri;
to nsIMsgMailNewsUrl.idl to maintain the getter functions, but there's no point since no one would call them.
Attachment #8994944 - Attachment description: 1478441-part1.patch - emegency surgery → 1478441-part1.patch - emergency surgery
Well, the other option is something similar to ORIGIN_IS_FULL_SPEC but that delegates some sort of complex checking to the URI impl.

And another option is to change mailbox: URIs to be nsIStandardURL, with per-message hostnames, and then just remove the ORIGIN_IS_FULL_SPEC thing and have them do security checks like any other hostname-based URI.
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/7ee9f9040488
Port 1228139: Replace use of nsIURIWithPrincipal, part 1, emergency measures. rs=bustage-fix
OK, I've just extinguished the fire, so now let's see what to do with the debris ;-(

Boris, thanks for comment #4, sadly I'm not an expert on any of that, but since the departure of Kent, the last man standing. So let's see. Looks like we already do ORIGIN_IS_FULL_SPEC, see:
https://searchfox.org/mozilla-central/rev/bdfd20ef30d521b57d5b6feeda71325e8b4cad66/netwerk/base/nsIProtocolHandler.idl#19
So I don't know what "something similar to ORIGIN_IS_FULL_SPEC" is meant to mean.

The next option is to change our mailbox: URIs, which are mostly like file: URIs to include a hostname? We (sadly) already have various formats (quoting from some old bugs):

mailbox://tbtest@example.com@server.de/Inbox?number=1
mailbox:///C:/Users/jorgk/AppData/Roaming/Thunderbird/Profiles/5gfhfw7f.POP%20tbtest/Mail/server.de/Inbox?number=1
and there's also
mailbox://nobody@Local%20Folders/xxx
and
mailbox:///C:/Users/jorgk/AppData/Local/Temp/nsmail-13.eml&number=0 for single messages.

Making any changes there would be a major operation.

So what's at the base of the <img crossorigin="anonymous" ...> we tried to solve in the first place?
I think I remember it now. Since the origin is the full spec, so with all the little bits and pieces Mailnews adds, there is no chance that Gecko can do any decent security checks. That's why we introduced the "principal spec" which was a normalised spec without the disturbing bits, and that was retrieved via nsIURIWithPrincipal. Anyway, something along those lines.

But even if I added a server/hostname, it would be hard to do for local folders. They would all get the same server, well, unless I invent a "per folder" server. Oh, you even talked about a "per-message hostname" in comment #4.

So something like:
mailbox://nobody@msg1.fake/C:/Users/jorgk/etc/Inbox?number=1
That's certainly a big ask.

And we're only talking mailbox: here? There are IMAP messages with imap:// URLs. They already have a server, but you don't want one message on the server to get access to other messages on the same server. So there you'd need a "real" server to address the message and a "fake" per-message hostname for security checking? Hmm. It was certainly more handy to be able to tell Gecko which spec to use for the checks.
> mailbox://nobody@msg1.fake/C:/Users/jorgk/etc/Inbox?number=1

Well.   You could do something like, using your examples from comment 6:

  mailbox://tbtest%40example.com%40server.de%2FInbox%3Fnumber%3D1
  mailbox://%2FC%3A%2FUsers%2Fjorgk%2FAppData%2FRoaming%2FThunderbird%2FProfiles%2F5gfhfw7f.POP%2520tbtest%2FMail%2Fserver.de%2FInbox%3Fnumber%3D1
  mailbox://nobody%40Local%2520Folders%2Fxxx

As in, take the entire bit after "//", or just the "principal spec" part, escape it, and use the result as the hostname.

> And we're only talking mailbox: here?

Presumably whichever things currently implement nsIURIWithPrincipal...  For imap:// there might be complication here, I agree.

I think we should see what Andrea says.
I thought I had submitted this....

> So I don't know what "something similar to ORIGIN_IS_FULL_SPEC" is meant to mean.

I mean another flag, that says "for this URI, ask it directly whether the two URIs are same-origin or not"  or some such.  This would presumably need a new interface such URIs would need to implement; whether that's something we really want is unclear.

> The next option is to change our mailbox: URIs, which are mostly like file: URIs to include a hostname?

Yes.  Fundamentally, the web security model is built around hostnames.  So the simplest way to get security boundaries is to make them look like hostnames.

> Making any changes there would be a major operation.

Sure.
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/37be8758d57e
Part 2: disable failing test test-image-display.js. rs=bustage-fix
Whiteboard: [Thunderbird-testfailure: Z all][Thunderbird-disabled-test][Thunderbird-temporary-fix]
Attached patch 1478441-M-C-part.patch - WIP (obsolete) — Splinter Review
I need to head out for a few hours, so I thought I'll present my idea as a WIP. I didn't get it compiled yet, somehow the derived class nsMsgMailNewsUrl doesn't appear to see the method to override it.

Anyway, perhaps something along these lines can work. No word from Andrea yet.
Attachment #8995174 - Flags: feedback?(bzbarsky)
Attached patch 1478441-M-C-part.patch (v2) (obsolete) — Splinter Review
I wrestled a bit with the compiler. The problem is that
#if defined(MOZ_THUNDERBIRD) || defined(MOZ_SUITE)
#define IS_ORIGIN_IS_FULL_SPEC_DEFINED 1
#endif
is in nsIProtocolHandler.idl and that's not included everywhere.

So for now I duplicated this. So far it compiles. Changing nsIURI.idl is no fun since it recompiles half the system.

I'll ask for review when I have the C-C part.
Assignee: nobody → jorgk
Attachment #8995174 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8995174 - Flags: feedback?(bzbarsky)
This is the C-C part. However, I can't test it since the program doesn't even start, not even with only the M-C patch applied. So those M-C changes are pretty destructive.

The debug console says:
[5900, Unnamed thread 193be60c800] WARNING: XPCOM objects created/destroyed from static ctor/dtor: file c:/mozilla-source/comm-central/xpcom/base/nsTraceRefcnt.cpp, line 209
[5900, Main Thread] WARNING: NS_ENSURE_SUCCESS_VOID(rv) failed with result 0x80520012: file c:/mozilla-source/comm-central/dom/base/nsFrameMessageManager.cpp, line 1381
[5900, Main Thread] WARNING: CheckLinkStatus called on main thread! No check performed. Assuming link is up, status is unknown.: file c:/mozilla-source/comm-central/netwerk/system/win32/nsNotifyAddrListener.cpp, line 710

What is so destructive about that I'm doing. Boris, could you take a look please.
Flags: needinfo?(bzbarsky)
That seems like a false alarm. Here are the equivalent lines when running without the M-C patch:

[10940, Main Thread] WARNING: CheckLinkStatus called on main thread! No check performed. Assuming link is up, status is unknown.: file c:/mozilla-source/comm-central/netwerk/system/win32/nsNotifyAddrListener.cpp, line 710
[10940, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80040111: file c:/mozilla-source/comm-central/netwerk/protocol/res/SubstitutingProtocolHandler.cpp, line 342
> What is so destructive about that I'm doing

Well, you're adding stuff to the nsIURI vtable that is in a C++ block, but xpconnect doesn't know about it.  That will make calls through xpconnect into any idl API that inherits from nsIURI jump off into lala-land.  That includes nsIMozIconURI and nsIURL.

That's my best guess for what's going wrong, at least.
Flags: needinfo?(bzbarsky)
Thanks, yes, that sounds plausible. So how to fix that damage? And what about the approach in general?
Don't change nsIURI.  Add a new interface or something and QI to it...
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #18)
> Don't change nsIURI.  Add a new interface or something and QI to it...
OK. So we basically add parts of what was removed back again. Call it nsIURIWithPrincipal? ;-) - Then I can just back out the C-C patches and everything will work again.
Again, please talk to Andrea.
This restores the bits we need.
Attachment #8995293 - Attachment is obsolete: true
Attachment #8995306 - Attachment is obsolete: true
Attachment #8995359 - Flags: review?(bzbarsky)
Fixed ugly variable name. Sorry 'bout the noise.
Attachment #8995359 - Attachment is obsolete: true
Attachment #8995359 - Flags: review?(bzbarsky)
Attachment #8995360 - Flags: review?(bzbarsky)
Typo in Mailnews :-( - Sorry again about more noise.
Attachment #8995361 - Flags: review?(bzbarsky)
Attachment #8995360 - Attachment is obsolete: true
Attachment #8995360 - Flags: review?(bzbarsky)
Comment on attachment 8995361 [details] [diff] [review]
1478441-restore-nsIURIWithPrincipal.patch (v1c)

Well, no word from Andrea. Maybe he'll react to a review request.
Attachment #8995361 - Flags: review?(amarchesini)
Comment on attachment 8995361 [details] [diff] [review]
1478441-restore-nsIURIWithPrincipal.patch (v1c)

Andrea should review this.
Attachment #8995361 - Flags: review?(bzbarsky)
Attachment #8995361 - Attachment is obsolete: true
Attachment #8995361 - Flags: review?(amarchesini)
Flags: needinfo?(amarchesini)
Attachment #8995442 - Flags: review?(amarchesini)
Note that there is already code in M-C to support Thunderbird's URLs:
https://searchfox.org/mozilla-central/search?q=IS_ORIGIN_IS_FULL_SPEC_DEFINED&case=false&regexp=false&path=

So restoring previously removed code protected with a conditional compile shouldn't be a problem, IMHO. Not having this code regresses bug 1347598. Fixing the issue any other way would entail significant effort and great risk.
Component: General → Networking
Product: MailNews Core → Core
Summary: Port 1228139: Replace use of nsIURIWithPrincipal → Restore parts of nsIURIWithPrincipal which was removed in bug 1228139
Version: 60 → 60 Branch
Version: 60 Branch → Trunk
Comment on attachment 8995442 [details] [diff] [review]
1478441-restore-nsIURIWithPrincipal.patch (v1c) - changed reviewer

Review of attachment 8995442 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/base/nsIURIWithPrincipal.idl
@@ +10,5 @@
> + * nsIURIWithPrincipal is implemented by URIs which are associated with a
> + * specific principal. This is exclusively used in comm-central's Mailnews module.
> + */
> +[scriptable, builtinclass, uuid(626a5c0c-bfd8-4531-8b47-a8451b0daa33)]
> +interface nsIURIWithPrincipal : nsISupports

Sorry, I was in PTO Thursday and Friday.
This patch re-introduce nsIURIWithPrincipal just in a couple of places.

You should probably restore these too:

https://dxr.mozilla.org/mozilla-beta/rev/fc0a1b63ccc727f9cb4ed424d3ac88e86529fc46/caps/NullPrincipal.cpp#184
https://dxr.mozilla.org/mozilla-beta/rev/fc0a1b63ccc727f9cb4ed424d3ac88e86529fc46/netwerk/base/nsNetUtil.cpp#2520

Probably Thunderbird doesn't use first-party-isolation. Ignore this:
https://dxr.mozilla.org/mozilla-beta/rev/fc0a1b63ccc727f9cb4ed424d3ac88e86529fc46/caps/OriginAttributes.cpp#67

There are 2 reasons why I wanted/want to remove nsIURIWithPrincipal:
1. in firefox, this is used only for blobURLs and I don't want to implement 'remote' blobURLs for content processes: a blobURL doesn't know its principal/blobImpl until it's opened for real.

2. We want to make nsIURI and nsIPrincipal thread-safe objects. nsIURI is partially thread-safe (the CTOR must be executed on the main-thread). nsIPrincipal is not thread-safe at all. It's easier to make them thread-safe if there is not an interface that 'links' the 2 objects.

Here my idea: it seems that you don't really care about the principal if you can have the origin as nsIURI. You need to extra part of the mailbox: URL and consider it as 'origin'. What about if you introduce something like:

interface nsIURIWithSpecialOrigin {
  readonly nsIURI origin;
}

This should not require a huge refactoring of how thunderbird deals with mailbox: URLs. Could it work?
I'm online on IRC if you want to discuss this issue more.
Attachment #8995442 - Flags: review?(amarchesini) → review-
Thank you for the detailed answer.

(In reply to Andrea Marchesini [:baku] from comment #28)
> You should probably restore these too:
I restored what made our test pass ;-)

> Here my idea: it seems that you don't really care about the principal if you
> can have the origin as nsIURI. You need to extra part of the mailbox: URL
> and consider it as 'origin'. ... Could it work?
Looks doable. I'll look into it during the "night shift" ;-)

> I'm online on IRC if you want to discuss this issue more.
Let me explore your idea.
Summary: Restore parts of nsIURIWithPrincipal which was removed in bug 1228139 → Introduce nsIURIWithSpecialOrigin for Thunderbird to replace nsIURIWithPrincipal which was removed in bug 1228139
Something like this? Should I still add the bits in NullPrincipal.cpp and nsNetUtil.cpp you poineted out in comment #28.
Attachment #8995442 - Attachment is obsolete: true
Attachment #8995912 - Flags: feedback?(amarchesini)
This is the C-C part. Compiles and works ;-) We should rename GetPrincipalSpec() to GetNormalizedSpec(), but that's another story.
Comment on attachment 8995912 [details] [diff] [review]
1478441-nsIURIWithSpecialOrigin.patch - WIP (v1)

Review of attachment 8995912 [details] [diff] [review]:
-----------------------------------------------------------------

Yes. Thanks.
Attachment #8995912 - Flags: feedback?(amarchesini) → feedback+
(In reply to Jorg K (GMT+2) from comment #30)
> Should I still add the bits in NullPrincipal.cpp and nsNetUtil.cpp you pointed out in comment #28?
(In reply to Jorg K (GMT+2) from comment #33)
> (In reply to Jorg K (GMT+2) from comment #30)
> > Should I still add the bits in NullPrincipal.cpp and nsNetUtil.cpp you pointed out in comment #28?

Yes. that is needed. In particular nsNetUtil.cpp where we check if 2 URLs can load each other.
This is the C-C part. It needs to be applied on to of a backout of the "emergency measures" landed as revision 7ee9f9040488. I did it this way so the transition from nsIURIWithPrincipal with nsIURIWithSpecialOrigin is clearly visible.
Attachment #8995915 - Attachment is obsolete: true
OK, I've added the parts in NullPrincipal.cpp and nsNetUtil.cpp. Hard to tell for me whether I got this right.

Here's a try run with the M-C and C-C patches applied:
https://hg.mozilla.org/try/rev/ae848c8f3284449011653d11d1ce2b3887c15ecb (M-C)
used by
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=5bb85f23de2234a175ae7860ea4609a4fa1e33d8
(The trick is in modifying .taskcluster.yml to point to that revision on M-C try.)
Attachment #8995912 - Attachment is obsolete: true
Attachment #8995961 - Flags: review?(amarchesini)
Attachment #8995948 - Flags: review?(amarchesini)
Comment on attachment 8995961 [details] [diff] [review]
1478441-nsIURIWithSpecialOrigin.patch - M-C part (v2)

Review of attachment 8995961 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/base/nsIURIWithSpecialOrigin.idl
@@ +17,5 @@
> +     * Special origin.
> +     */
> +    readonly attribute nsIURI origin;
> +};
> +

no extra line here.
Attachment #8995961 - Flags: review?(amarchesini) → review+
Comment on attachment 8995948 [details] [diff] [review]
1478441-use-nsIURIWithSpecialOrigin.patch - C-C part (v2)

Review of attachment 8995948 [details] [diff] [review]:
-----------------------------------------------------------------

good. Thanks!
Attachment #8995948 - Flags: review?(amarchesini) → review+
Comment on attachment 8995961 [details] [diff] [review]
1478441-nsIURIWithSpecialOrigin.patch - M-C part (v2)

Review of attachment 8995961 [details] [diff] [review]:
-----------------------------------------------------------------

You want to change nsContentUtil::GetUTFOrigin (2 methods). Separate patch?
I've modified
  nsContentUtils::GetUTFOrigin(nsIURI* aURI, nsAString& aOrigin).

The new IDL file accidentally had Windows CRLF line endings in the previous patch, so the compile totally failed. Here we go again:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=d3b50ffce01cf87d9630ece34974522951b2aed9
Attachment #8995961 - Attachment is obsolete: true
Attachment #8996021 - Flags: review?(amarchesini)
Moved variable down a bit, sorry about the noise.
Attachment #8996021 - Attachment is obsolete: true
Attachment #8996021 - Flags: review?(amarchesini)
Attachment #8996023 - Flags: review?(amarchesini)
More noise ;-( - I've been thinking whether those new hunks should do some error checking, but since most of the functions to which I'm adding don't return a status, checking the origin URI or checking the status is the same.

In nsContentUtils::GetUTFOrigin() returns a result, so I'm checking the status there.
Attachment #8996023 - Attachment is obsolete: true
Attachment #8996023 - Flags: review?(amarchesini)
Attachment #8996035 - Flags: review?(amarchesini)
So this is the new unreviewed bit that wasn't in (v2) which had r+:
https://bugzilla.mozilla.org/attachment.cgi?oldid=8995961&action=interdiff&newid=8996035&headers=1

The try run is "green" BTW, so this isn't doing any damage.
Changed "normalised" (British English) to "normalized" (US English) for consistency with the function name.
Attachment #8995948 - Attachment is obsolete: true
Attachment #8996041 - Flags: review+
Comment on attachment 8996035 [details] [diff] [review]
1478441-nsIURIWithSpecialOrigin.patch - M-C part (v3c)

Review of attachment 8996035 [details] [diff] [review]:
-----------------------------------------------------------------

::: caps/NullPrincipal.cpp
@@ +194,5 @@
> +      RefPtr<BasePrincipal> principal = BasePrincipal::CreateCodebasePrincipal(origin, attrs);
> +      return principal == this;
> +    }
> +  }
> +#endif

Actually, this block is wrong, because you are creating a new principal and that is different than 'this' for sure.
Remove this block. In general, you don't have nullprincipal for mailbox: URL, right?

::: dom/base/nsContentUtils.cpp
@@ +6350,5 @@
> +  // Check if either URI has a special origin.
> +  nsCOMPtr<nsIURIWithSpecialOrigin> uriWithSpecialOrigin = do_QueryInterface(aURI);
> +  if (uriWithSpecialOrigin) {
> +    nsCOMPtr<nsIURI> origin;
> +    nsresult rv2 = uriWithSpecialOrigin->GetOrigin(getter_AddRefs(origin));

why rv2? maybe add: nsresult rv; at the beginning of the method and use rv here and at line 6362.

::: netwerk/base/nsNetUtil.cpp
@@ +2487,5 @@
> +    // Check if either URI has a special origin.
> +    nsCOMPtr<nsIURI> origin;
> +    nsCOMPtr<nsIURIWithSpecialOrigin> uriWithSpecialOrigin = do_QueryInterface(sourceBaseURI);
> +    if (uriWithSpecialOrigin) {
> +      uriWithSpecialOrigin->GetOrigin(getter_AddRefs(origin));

nsresult rv = ...
if (NS_WARN_IF(NS_FAILED(rv))) {
  return false;
}

MOZ_ASSERT(origin);

@@ +2488,5 @@
> +    nsCOMPtr<nsIURI> origin;
> +    nsCOMPtr<nsIURIWithSpecialOrigin> uriWithSpecialOrigin = do_QueryInterface(sourceBaseURI);
> +    if (uriWithSpecialOrigin) {
> +      uriWithSpecialOrigin->GetOrigin(getter_AddRefs(origin));
> +      if (origin)

{}

@@ +2493,5 @@
> +        sourceBaseURI = origin;
> +    }
> +    uriWithSpecialOrigin = do_QueryInterface(targetBaseURI);
> +    if (uriWithSpecialOrigin) {
> +      uriWithSpecialOrigin->GetOrigin(getter_AddRefs(origin));

same here.
Attachment #8996035 - Flags: review?(amarchesini) → review+
Thanks. Following the same scheme, shouldn't the code in ContentPrincipal.cpp be like this for consistency?

+#if defined(MOZ_THUNDERBIRD) || defined(MOZ_SUITE)
+  nsCOMPtr<nsIURIWithSpecialOrigin> uriWithSpecialOrigin = do_QueryInterface(aURI);
+  if (uriWithSpecialOrigin) {
+    nsCOMPtr<nsIURI> origin;
+    rv = uriWithSpecialOrigin->GetOrigin(getter_AddRefs(origin));
+    if (NS_WARN_IF(NS_FAILED(rv))) {
+      return false;
+    }
+    MOZ_ASSERT(origin);
+    OriginAttributes attrs;
+    RefPtr<BasePrincipal> principal = BasePrincipal::CreateCodebasePrincipal(origin, attrs);
+    return nsIPrincipal::Subsumes(principal);
+  }
+#endif

And in BasePrincipal.cpp:

+#if defined(MOZ_THUNDERBIRD) || defined(MOZ_SUITE)
+  nsCOMPtr<nsIURIWithSpecialOrigin> uriWithSpecialOrigin = do_QueryInterface(aURI);
+  if (uriWithSpecialOrigin) {
+    nsCOMPtr<nsIURI> origin;
+    rv = uriWithSpecialOrigin->GetOrigin(getter_AddRefs(origin));
+    if (NS_WARN_IF(NS_FAILED(rv))) {
+      return nullptr;
+    }
+    MOZ_ASSERT(origin);
+    OriginAttributes attrs;
+    RefPtr<BasePrincipal> principal = CreateCodebasePrincipal(origin, attrs);
+    return principal.forget();
+  }
+#endif

Not ideal to return a nullptr there, but there's a NS_ENSURE_SUCCESS(rv, nullptr); later in the code.

Thanks for your patience!
Flags: needinfo?(amarchesini)
Addressed review issues from comment #45. Carrying forward :baku's r+.
Attachment #8996035 - Attachment is obsolete: true
Attachment #8996399 - Flags: review+
Here's a version which changes the error handling as per my comment #46.

Andreas, please do an interdiff to v4 and then pick the one you prefer.
Flags: needinfo?(amarchesini)
Attachment #8996403 - Flags: review?(amarchesini)
Sorry, Andrea, no "s".
Priority: -- → P5
Whiteboard: [Thunderbird-testfailure: Z all][Thunderbird-disabled-test][Thunderbird-temporary-fix] → [necko-triaged][Thunderbird-testfailure: Z all][Thunderbird-disabled-test][Thunderbird-temporary-fix]
Comment on attachment 8996403 [details] [diff] [review]
1478441-nsIURIWithSpecialOrigin.patch - M-C part (v5)

Review of attachment 8996403 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!
Attachment #8996403 - Flags: review?(amarchesini) → review+
Attachment #8996399 - Attachment is obsolete: true
Dear Sheriff,
please check in "1478441-nsIURIWithSpecialOrigin.patch - M-C part (v5)". I'll handle the C-C part myself.
Thanks you.

P.S.: I had to repeat the try for TB, successful here:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=a2f531fb566c374f96fd7182a79f6385b610d14b
Pushed by dluca@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/01533e166085
Introduce nsIURIWithSpecialOrigin needed for Thunderbird. r=baku
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/01533e166085
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/071b02e0acfd
Backed out changeset 37be8758d57e to re-enable tests. a=backout
https://hg.mozilla.org/comm-central/rev/d5c25d3deed9
Backed out emergency measures from changeset 7ee9f9040488. a=backout
https://hg.mozilla.org/comm-central/rev/1c28beaa36fb
Replace nsIURIWithPrincipal with nsIURIWithSpecialOrigin. r=baku
You need to log in before you can comment on or make changes to this bug.