Closed Bug 1316256 Opened 8 years ago Closed 8 years ago

content of <noscript> tag sometimes not show in message pane - TEST-UNEXPECTED-FAIL | C:\slave\test\build\tests\mozmill\content-policy\test-js-content-policy.js

Categories

(Core :: DOM: Security, defect)

Product:
Core
Component:
DOM: Security
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox51 --- unaffected
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: jorgk-bmo, Assigned: jorgk-bmo)

References

Details

(Keywords: intermittent-failure, regression)

Attachments

(3 files, 6 obsolete files)

Test mail showing the problem ... or not
750 bytes, text/plain
Details
1316256-alternative.patch (CK v1).
1.84 KB, patch
jorgk-bmo
: review+
jcristau
: approval-mozilla-aurora+
Details | Diff | Splinter Review
webrequest fix
1.15 KB, patch
jorgk-bmo
: review+
jcristau
: approval-mozilla-aurora+
Details | Diff | Splinter Review 
First seen Tue Nov 8, 2016, 22:46:39

https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=e4ab5c338d81d9c1e457364141e9aeac5fcfe422

INFO -  SUMMARY-UNEXPECTED-FAIL | C:\slave\test\build\tests\mozmill\content-policy\test-js-content-policy.js | test-js-content-policy.js::test_jsContentPolicy
INFO -    EXCEPTION: noscript display should be 'inline'; display=none
INFO -      at: test-js-content-policy.js line 110
INFO -         checkJsInMail test-js-content-policy.js:110 11
INFO -         test_jsContentPolicy test-js-content-policy.js:255 3
INFO -         Runner.prototype.wrapper frame.js:585 9
INFO -         Runner.prototype._runTestModule frame.js:655 9
INFO -         Runner.prototype.runTestModule frame.js:701 3
INFO -         Runner.prototype.runTestDirectory frame.js:525 7
INFO -         runTestDirectory frame.js:707 3
INFO -         Bridge.prototype._execFunction server.js:179 10
INFO -         Bridge.prototype.execFunction server.js:183 16
INFO -         Session.prototype.receive server.js:283 3
INFO -         AsyncRead.prototype.onDataAvailable server.js:88 3

mozmake SOLO_TEST=content-policy/test-js-content-policy.js mozmill-one passes locally.
It's possible to reproduce this locally after all - you can even see the noscript textcontent failing to appear. Not obvious what the cause is...
Regression window m-c 680094d957919bd757685e3de5fe244a5e038658 - 783356f1476eafd8e4d6fa5f3919cf6167e84f8d
Nope.
Last good: aea5b4c3d165dcde027b3b6551b146a567
First bad: 680094d957919bd757685e3de5fe244a5e

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=aea5b4c3d165&tochange=680094d95791

Sorry about not being able to reproduce it locally: I didn't have a fresh M-C, doh :-(
OK, I looked at this a little further.

test-js-content-policy.js runs a few texts, but the first one fails already:
checkJsInMail();

This test displays this message:
<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
<html>
<head>

<meta http-equiv="content-type" content="text/html; charset=ISO-8859-1">
</head>
<body bgcolor="#ffffff" text="#000000">
this is a test<big><big><big> stuff
<br><br>
</big></big></big>
<noscript>
hello, this content is noscript!
</noscript>
<script>
var jsIsTurnedOn = true;
</script>

</body>
</html>

If "noscript" is set, the text |hello, this content is noscript!| should show in the message pane. But it doesn't, hence the test result display=none. I can only see |this is a test stuff|.

So something in M-C has changed the processing of the <noscript> tag.

I tried in FF 49 (current at time of writing) and the page is displayed differently depending on whether javascript.enabled is true or false.
(In reply to Jorg K (GMT+1) from comment #3)
> Nope.
> Last good: aea5b4c3d165dcde027b3b6551b146a567
> First bad: 680094d957919bd757685e3de5fe244a5e 

This is wrong, the test passes with 680094d957919bd757685e3de5fe244a5e ;)
The strange thing is that if you display a message with the content in Daily or a local build, the "noscript" text *is* displayed. It's a mystery why it's not displayed when the message is displayed during the test.
Damn, I clicked onto the wrong "B", grr. So let's see then:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=680094d95791&tochange=783356f1476e
Aleth, thanks for pointing out the *correct* regression range. Since it's a display problem, I'm tipping that bug 1308889 is our foe here. If backed out 57d473c3ca22 locally and now the test passes again.

Christoph, do you have any hints? In our Mozmill test the "noscript" text isn't rendered. Adding Boris as well, since he's helped us before in similar situations.
Flags: needinfo?(ckerschb)
Flags: needinfo?(bzbarsky)
Blocks: 1308889
Keywords: regression
(In reply to Jorg K (GMT+1) from comment #6)
> The strange thing is that if you display a message with the content in Daily
> or a local build, the "noscript" text *is* displayed. It's a mystery why
> it's not displayed when the message is displayed during the test.

Do you mean this works when you look at the same email in Daily (outside the mozmill context)?
What's surprising here is that while the noscript tag isn't rendered, JS appears to be disabled (as it should be), which is inconsistent.
(In reply to aleth [:aleth] from comment #9)
> (In reply to Jorg K (GMT+1) from comment #6)
> > The strange thing is that if you display a message with the content in Daily
> > or a local build, the "noscript" text *is* displayed. It's a mystery why
> > it's not displayed when the message is displayed during the test.
> Do you mean this works when you look at the same email in Daily (outside the
> mozmill context)?

Please try yourself with the attached message. Opening the message from the file does NOT show the "noscript" part. Importing it into a local folder sometimes does and sometimes does NOT show the "noscript" part. It's completely confusing. Looks like the test hits the cases where it does not show.
Summary: TEST-UNEXPECTED-FAIL | C:\slave\test\build\tests\mozmill\content-policy\test-js-content-policy.js → content of <noscript> tag sometimes not show in message pane - TEST-UNEXPECTED-FAIL | C:\slave\test\build\tests\mozmill\content-policy\test-js-content-policy.js
Looks like if you repair the local folder, the "noscript" part doesn't show.
Other experiments. If you open the message to another tab, close that tab and return to the message preview, the text will typically show even if it didn't show before. Also clicking onto another message and then returning to the first will make the text show. Aleth, just give it a try with the test message. It appears that on "first showing", the "noscript" part doesn't appear.

Maybe some sort of internal timing problem?
> If backed out 57d473c3ca22 locally and now the test passes again.

I assume you've verified this carefully?  Because there are multiple regression ranges listed in this bug, which are then claimed to not be right, etc...

I don't see how https://hg.mozilla.org/mozilla-central/rev/57d473c3ca22 would affect noscript handling offhand.

If you can reproduce this, can you breakpoint in nsLayoutUtils::ShouldUseNoScriptSheet and see what it returns for the relevant document and why?
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #13)
> > If backed out 57d473c3ca22 locally and now the test passes again.
> I assume you've verified this carefully?
I backed it out, started working, I backed out the backout, failed again.

> Because there are multiple
> regression ranges listed in this bug, which are then claimed to not be
> right, etc...
We're working on TB and sadly I picked the wrong M-C version by clicking on the wrong "B" in Treeherder. Different platforms got build with different M-C versions in the same C-C push since the builds were triggered at different points in time. My mistake. I'm pretty sure I got it right now and Aleth has confirmed.

> I don't see how https://hg.mozilla.org/mozilla-central/rev/57d473c3ca22
> would affect noscript handling offhand.
Neither do I. The funny thing is that the "noscript" part typically don't show on first viewing of the message, but later is does.

> If you can reproduce this, can you breakpoint in
> nsLayoutUtils::ShouldUseNoScriptSheet and see what it returns for the
> relevant document and why?
I'll get right to it. Thanks for the hint.
> I backed it out, started working, I backed out the backout, failed again.

Alright.  So the other thing I would be interested in is what the principal of the relevant document ends up being both before and after that changeset.
Boris, I'm terribly sorry, but I'm neither a document nor docshell nor security expert. I looked at nsLayoutUtils::ShouldUseNoScriptSheet(nsIDocument* aDocument) but I wouldn't know how to dump out the information you need.

Once upon a time Christoph gave me some debug patches in bug 1246500. I don't need a patch, just some hints to dump out what you need to see. You still want to dump this out in ShouldUseNoScriptSheet()?

Sorry once again. Once upon a time you also worked on e-mail related issues (as I can see from reading old bugs), you'll know that it's a wide field. I just know better about multipart messages (and even Core::Editor) that what's required here.
Flags: needinfo?(bzbarsky)
The initial thing I wanted to see is this.  In ShouldUseNoScriptSheet():

  nsCAutoString spec;
  aDocument->GetDocumentURI()->GetSpec(spec);

then dump out spec.get() (which is a char*) and the boolean value the function ends up returning for that document.  If you see the document that has the noscript tag in there and can recognize it based on the URL, that will tell you what boolean is being returned.

At that point you can look into also dumping the following:

1)  aDocument->NodePrincipal()->IsSystemPrincipal()
2)  aDocument->NodePrincipal()->IsCodebasePrincipal()
3)  aDocument->NodePrincipal()->IsNullPrincipal()

and if IsCodebasePrincipal() is true, then:

  nsCOMPtr<nsIURI> principalURI;
  aDocument->NodePrincipal()->GetURI(getter_AddRefs(principalURI));
  nsCAutoString principalURISpec;
  principalURI->GetSpec(principalURISpec);

and then see what that string is.
Flags: needinfo?(bzbarsky)
Thanks so much. I got this to compile - gotta love the three methods *Is*Principal() ;-(

/* static */ bool
nsLayoutUtils::ShouldUseNoScriptSheet(nsIDocument* aDocument)
{
  nsAutoCString spec;
  aDocument->GetDocumentURI()->GetSpec(spec);
  printf("=== ShouldUseNoScriptSheet: doc=|%s|\n", spec.get());
  
  bool sp = nsContentUtils::IsSystemPrincipal(aDocument->NodePrincipal());
  bool cp = BasePrincipal::Cast(aDocument->NodePrincipal())->IsCodebasePrincipal();
  bool np = aDocument->NodePrincipal()->GetIsNullPrincipal();
  printf("=== ShouldUseNoScriptSheet: Sys/Code/Null Principal: %d/%d/%d\n",
  sp?1:0, cp?1:0, np?1:0);
  if (cp) {
    nsCOMPtr<nsIURI> principalURI;
    aDocument->NodePrincipal()->GetURI(getter_AddRefs(principalURI));
    nsAutoCString principalURISpec;
    principalURI->GetSpec(principalURISpec);
    printf("=== ShouldUseNoScriptSheet: CodebasePrincipal=|%s|\n", principalURISpec.get());
  }

  // also handle the case where print is done from print preview
  // see bug #342439 for more details
  if (aDocument->IsStaticDocument()) {
    aDocument = aDocument->GetOriginalDocument();
  }
  bool r = aDocument->IsScriptEnabled();
  printf("=== ShouldUseNoScriptSheet returns %d\n", r?1:0);
  return r;
}

Opening the message from a file:
=== ShouldUseNoScriptSheet: doc=|mailbox:///D:/Desktop/noscript.eml?type=application/x-message-display&number=0|
=== ShouldUseNoScriptSheet: Sys/Code/Null Principal: 0/1/0
=== ShouldUseNoScriptSheet: CodebasePrincipal=|mailbox:///D:/Desktop/noscript.eml?type=application/x-message-display&number=0|
=== ShouldUseNoScriptSheet returns 1
The "noscript" part does *NOT* show.

First display from a folder in the preview pane:
=== ShouldUseNoScriptSheet: doc=|mailbox:///C:/Users/jorgk/AppData/Roaming/Thunderbird/Profiles/testing.testing/Mail/Local%20Folders/Trash?number=1|
=== ShouldUseNoScriptSheet: Sys/Code/Null Principal: 0/1/0
=== ShouldUseNoScriptSheet: CodebasePrincipal=|mailbox:///C:/Users/jorgk/AppData/Roaming/Thunderbird/Profiles/testing.testing/Mail/Local%20Folders/Trash?number=1|
=== ShouldUseNoScriptSheet returns 1
The "noscript" part does *NOT* show.

Opening the message in a tab:
=== ShouldUseNoScriptSheet: doc=|mailbox:///C:/Users/jorgk/AppData/Roaming/Thunderbird/Profiles/testing.testing/Mail/Local%20Folders/Trash?number=1|
=== ShouldUseNoScriptSheet: Sys/Code/Null Principal: 0/1/0
=== ShouldUseNoScriptSheet: CodebasePrincipal=|mailbox:///C:/Users/jorgk/AppData/Roaming/Thunderbird/Profiles/testing.testing/Mail/Local%20Folders/Trash?number=1|
=== ShouldUseNoScriptSheet returns 0
Not the "noscript" part shows.

Back to the preview pane where the text shows now:
=== ShouldUseNoScriptSheet: doc=|mailbox:///C:/Users/jorgk/AppData/Roaming/Thunderbird/Profiles/testing.testing/Mail/Local%20Folders/Trash?number=1|
=== ShouldUseNoScriptSheet: Sys/Code/Null Principal: 0/1/0
=== ShouldUseNoScriptSheet: CodebasePrincipal=|mailbox:///C:/Users/jorgk/AppData/Roaming/Thunderbird/Profiles/testing.testing/Mail/Local%20Folders/Trash?number=1|
=== ShouldUseNoScriptSheet returns 0

As I said before: On the very same message, you get random results, typically the first display doesn't show the "noscript" part.

Sorry about the NI, I don't know how closely you follow your bugmail.
Flags: needinfo?(bzbarsky)
(In reply to Jorg K (GMT+1) from comment #18)
> === ShouldUseNoScriptSheet returns 0
> NoW the "noscript" part shows.
(Most annoying thing about English is the closeness of "not" and "now".)
OK, thanks.

So aDocument->IsStaticDocument() is always false in your cases, I assume.

Why is script expected to be disabled for this document?  Is the docshell's allowJavascript being set to false, or something else?

In nsGlobalWindow::SetNewDocument, can you log what the document URI is (so you can tell when it's your mailbox:// URI) and what GetDocShell()->GetCanExecuteScripts(); ends up returning in that case?
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #20)
> So aDocument->IsStaticDocument() is always false in your cases, I assume.
Yes, I checked that with more debug.

> Why is script expected to be disabled for this document?  Is the docshell's
> allowJavascript being set to false, or something else?
Yes
https://dxr.mozilla.org/comm-central/rev/2cc4df8526b144b85685c817aa5ca3d1fb637de7/mailnews/base/src/nsMsgContentPolicy.cpp#739
and I'm dumping this out as well, see below. Turns out that this is never called.

> In nsGlobalWindow::SetNewDocument, can you log what the document URI is (so
> you can tell when it's your mailbox:// URI) and what
> GetDocShell()->GetCanExecuteScripts(); ends up returning in that case?
See debug below.

File case, "noscript" part not showing:
=== SetNewDocument: doc=|mailbox:///D:/Desktop/noscript.eml?type=application/x-message-display&number=0|
=== SetNewDocument: GetCanExecuteScripts 1
=== ShouldUseNoScriptSheet: doc=|mailbox:///D:/Desktop/noscript.eml?type=application/x-message-display&number=0|
=== ShouldUseNoScriptSheet: Sys/Code/Null Principal: 0/1/0
=== ShouldUseNoScriptSheet: CodebasePrincipal=|mailbox:///D:/Desktop/noscript.eml?type=application/x-message-display&number=0|
=== ShouldUseNoScriptSheet returns 1
=== nsMsgContentPolicy::ShouldLoad: URI=|chrome://messagebody/skin/messageBody.css|
and more calls to ShouldLoad() on other CSS and PNG files.

Message case not showing:
=== SetNewDocument: doc=|mailbox:///C:/Users/jorgk/AppData/Roaming/Thunderbird/Profiles/testing.testing/Mail/Local%20Folders/Trash?number=1|
=== SetNewDocument: GetCanExecuteScripts 1
=== ShouldUseNoScriptSheet: doc=|mailbox:///C:/Users/jorgk/AppData/Roaming/Thunderbird/Profiles/testing.testing/Mail/Local%20Folders/Trash?number=1|
=== ShouldUseNoScriptSheet: Sys/Code/Null Principal: 0/1/0
=== ShouldUseNoScriptSheet: CodebasePrincipal=|mailbox:///C:/Users/jorgk/AppData/Roaming/Thunderbird/Profiles/testing.testing/Mail/Local%20Folders/Trash?number=1|
=== ShouldUseNoScriptSheet returns 1
=== nsMsgContentPolicy::ShouldLoad: URI=|chrome://messagebody/skin/messageBody.css|
and more calls to ShouldLoad() on other CSS and PNG files.

Case showing:
=== SetNewDocument: doc=|mailbox:///C:/Users/jorgk/AppData/Roaming/Thunderbird/Profiles/testing.testing/Mail/Local%20Folders/Trash?number=1|
=== SetNewDocument: GetCanExecuteScripts 0
=== ShouldUseNoScriptSheet: doc=|mailbox:///C:/Users/jorgk/AppData/Roaming/Thunderbird/Profiles/testing.testing/Mail/Local%20Folders/Trash?number=1|
=== ShouldUseNoScriptSheet: Sys/Code/Null Principal: 0/1/0
=== ShouldUseNoScriptSheet: CodebasePrincipal=|mailbox:///C:/Users/jorgk/AppData/Roaming/Thunderbird/Profiles/testing.testing/Mail/Local%20Folders/Trash?number=1|
=== ShouldUseNoScriptSheet returns 0
=== nsMsgContentPolicy::ShouldLoad: URI=|chrome://messagebody/skin/messageBody.css|
and more calls to ShouldLoad() on other CSS and PNG files.

Observation:
nsMsgContentPolicy::ShouldLoad() is never called for the whole document, only the CSS and images it loads, and therefore SetDisableItemsOnMailNewsUrlDocshells() is never called and also SetAllowJavascript() is not called.

I can see how the debug looks like when I back out 57d473c3ca22.
With 57d473c3ca22 backed out, I see this debug in my "open from file" case (first case):

=== SetNewDocument: doc=|mailbox:///D:/Desktop/noscript.eml?type=application/x-message-display&number=0|
=== SetNewDocument: GetCanExecuteScripts 0
=== ShouldUseNoScriptSheet: doc=|mailbox:///D:/Desktop/noscript.eml?type=application/x-message-display&number=0|
=== ShouldUseNoScriptSheet: Sys/Code/Null Principal: 0/1/0
=== ShouldUseNoScriptSheet: CodebasePrincipal=|mailbox:///D:/Desktop/noscript.eml?type=application/x-message-display&number=0|
=== ShouldUseNoScriptSheet returns 0
=== nsMsgContentPolicy::ShouldLoad: URI=|chrome://messagebody/skin/messageBody.css|

So straight away, GetCanExecuteScripts() returns 0.

No calls to our Mailnews stuff like SetDisableItemsOnMailNewsUrlDocshells() or consequently SetAllowJavascript():
https://dxr.mozilla.org/comm-central/rev/2cc4df8526b144b85685c817aa5ca3d1fb637de7/mailnews/base/src/nsMsgContentPolicy.cpp#688

So rev 57d473c3ca22 changed something so that now JS seems to be enabled when it was previously not enabled. The funny thing is that now it's enabled the "first time 'round" as comment #21 shows.

Where do we go from here?
Flags: needinfo?(bzbarsky)
> https://dxr.mozilla.org/comm-central/rev/2cc4df8526b144b85685c817aa5ca3d1fb637de7/mailnews/base/src/nsMsgContentPolicy.cpp#739

Uh... now we're getting somewhere.  So you mutate the docshell's "allow javascript" boolean from _inside_ a content policy's shouldLoad???  That's a really bizarre way to use that boolean; it's not really meant to be used that way at all.  :(

> No calls to our Mailnews stuff like SetDisableItemsOnMailNewsUrlDocshells()

Those calls would happen _before_ the SetNewDocument call for mailbox://whatever.  Can you please double-check again whether you get a SetDisableItemsOnMailNewsUrlDocshells call for the relevant mailbox:// URI with 57d473c3ca22 backed out, _before_ the corresponding SetNewDocument?

At this point my hypothesis is that 57d473c3ca22 changed what triggering principal is used for the mailbox:// load from something (what?) to system.  And for system triggering principals content policies are not called and hence ShouldLoad would not be called and you would not get to run SetDisableItemsOnMailNewsUrlDocshells.  It would be good to get that confirmed, e.g. by logging what aTriggeringPrincipal is in nsDocShell::DoURILoad for the various URIs coming through.  If it's confirmed, I would like to know what the triggering principal was before the changes from 57d473c3ca22.

Once we know that, we can try to figure out whether the behavior change 57d473c3ca22 made is correct or not, and if it's correct what mailnews should actually be doing.  Because this business of "toggle the docshell state in ShouldLoad" is really fragile and kinda broken; there should really be a better way.  For example, we could define some interface for a channel to directly provide sandbox flags, or a CSP that then lets you set sandbox flags, and mailnews could do that as needed...
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #23)
> Those calls would happen _before_ the SetNewDocument call for
> mailbox://whatever.  Can you please double-check again whether you get a
> SetDisableItemsOnMailNewsUrlDocshells call for the relevant mailbox:// URI
> with 57d473c3ca22 backed out, _before_ the corresponding SetNewDocument?
Sorry, there is so much DOM window debugging that it's hard to see. My mistake. Here we go with 57d473c3ca22 backed out.

=== nsMsgContentPolicy::ShouldLoad: URI=|mailbox:///D:/Desktop/noscript.eml?type=application/x-message-display&number=0|
=== nsMsgContentPolicy::ShouldLoad calling SetDisableItemsOnMailNewsUrlDocshells
=== SetDisableItemsOnMailNewsUrlDocshells: URI=|mailbox:///D:/Desktop/noscript.eml?type=application/x-message-display&number=0|
=== SetDisableItemsOnMailNewsUrlDocshells: Calling SetAllowJavascript(false)
=== SetNewDocument: doc=|mailbox:///D:/Desktop/noscript.eml?type=application/x-message-display&number=0|
=== SetNewDocument: GetCanExecuteScripts 0

So now you want the triggering principal?

Mailnews' nsMsgContentPolicy::ShouldLoad() has an aRequestPrincipal, would that be enough?

I can get aTriggeringPrincipal in nsDocShell::DoURILoad in a minute.
> Mailnews' nsMsgContentPolicy::ShouldLoad() has an aRequestPrincipal, would that be enough?

Yes.
OK, with 57d473c3ca22 backed out:

=== nsMsgContentPolicy::ShouldLoad: URI=|mailbox:///D:/Desktop/noscript.eml?type=application/x-message-display&number=0|
=== nsMsgContentPolicy::ShouldLoad: Request Principal is NULL
=== nsMsgContentPolicy::ShouldLoad calling SetDisableItemsOnMailNewsUrlDocshells
=== SetDisableItemsOnMailNewsUrlDocshells: URI=|mailbox:///D:/Desktop/noscript.eml?type=application/x-message-display&number=0|
=== SetDisableItemsOnMailNewsUrlDocshells: Calling SetAllowJavascript(false)
=== nsDocShell::DoURILoad: URI=|mailbox:///D:/Desktop/noscript.eml?type=application/x-message-display&number=0|
=== nsDocShell::DoURILoad: Triggering Principal is NULL
=== SetNewDocument: doc=|mailbox:///D:/Desktop/noscript.eml?type=application/x-message-display&number=0|
=== SetNewDocument: GetCanExecuteScripts 0

Looks like Request/Triggering Principal is NULL.

So now back out the back-out to see what happens with 57d473c3ca22 in place?
> Looks like Request/Triggering Principal is NULL.

Ah.  Yeah, that definitely got changed.  We used to let null through until DoURILoad, then change it to referrer or system if no referrer.  Now we do that earlier, but that changes the ShouldLoad behavior.  I assume in this case there is no referrer, so triggering principal ends up being system.

The behavior change for content policies is a bit unfortunate.  At the same time, this would automatically happen as we pushed the "tell us who is really doing the load" thing further up the callstack, which we want to do.

The real issue here is that mailnews is doing things other than just giving a "yes/no" answer from ShouldLoad, and we should really fix that.  Does it do things other than disabling script/plugins?  Does it ever need to _enable_ those on a subframe of a docshell that it disables them on?

(Also, I think now we understand what happens with the new behavior: ShouldLoad is no longer disabling script on the docshell, but script still gets disabled at _some_ point, just too late to affect the noscript stylesheet setup.)
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #27)
> Ah.  Yeah, that definitely got changed.  We used to let null through until
> DoURILoad, then change it to referrer or system if no referrer.  Now we do
> that earlier, but that changes the ShouldLoad behavior.  I assume in this
> case there is no referrer, so triggering principal ends up being system.
See debug below.

> The real issue here is that mailnews is doing things other than just giving
> a "yes/no" answer from ShouldLoad, and we should really fix that.  Does it
> do things other than disabling script/plugins?  Does it ever need to
> _enable_ those on a subframe of a docshell that it disables them on?
Look, I'm just the poor guy who's trying to keep the tree green. I'll look into it, but I think the answer is "no" to the last question (enable again).

Here's the debug with 57d473c3ca22 back in place:
=== nsDocShell::DoURILoad: URI=|mailbox:///D:/Desktop/noscript.eml?type=application/x-message-display&number=0|
=== nsDocShell::DoURILoad: Triggering Principal Sys/Code/Null Principal: 1/0/0
=== SetNewDocument: doc=|mailbox:///D:/Desktop/noscript.eml?type=application/x-message-display&number=0|
=== SetNewDocument: GetCanExecuteScripts 1

As predicted, nsDocShell::DoURILoad now called with a system principal.

No calls to |nsMsgContentPolicy::ShouldLoad: URI=|mailbox:...| or SetDisableItemsOnMailNewsUrlDocshells() or SetAllowJavascript().

You said "The real issue here is that mailnews is doing things other than just giving a "yes/no" answer from ShouldLoad", but ShouldLoad isn't even called any more on the mailbox URL in question.

Where do we go from here? And BTW, many thanks for your help so far.
Flags: needinfo?(bzbarsky)
> but ShouldLoad isn't even called any more on the mailbox URL in question

Right, ShouldLoad is never called for system triggering principals.

> Where do we go from here? 

Good question.  Tanvi, do you have time to think about this, given that Christoph is out for the moment?  The basic issue is that we used to do content policy checks in docshell _before_ we decided what our triggering principal would be, and in particular passed a null (in the nullptr sense, not nullprincipal sense) principal to the content policy check.  This meant that in all sorts of "default load from chrome" cases we would invoke content policies.

With the changes in bug 1308889 we now compute the triggering principal earlier, and it has to be system to pass CheckLoadURI checks, but that means content policies get bypassed.

Do we want to do something to restore the old content policy behavior?  Or should we go ahead with the new content policy behavior?  In the latter case, mailnews will need some API to provide sandbox flags, or a CSP that can set sandbox flags, off some non-httpchannel interface...
Flags: needinfo?(bzbarsky) → needinfo?(tanvi)
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #29)
> > but ShouldLoad isn't even called any more on the mailbox URL in question
> Right, ShouldLoad is never called for system triggering principals.
Please excuse my ignorant question which I'm asking just for my own education:
In comment #26 we saw that ShouldLoad() was called before nsDocShell::DoURILoad() and SetNewDocument(). Who made this call to ShouldLoad() with request principal NULL (nullptr)? That caller must now have "promoted" NULL to 'system' and not call ShouldLoad() at all any more.
>  Who made this call to ShouldLoad() with request principal NULL (nullptr)?

nsDocShell::InternalLoad.  

> That caller must now have "promoted" NULL to 'system'

Indeed.  The code to do that was hoisted up from DoURILoad to InternalLoad.
> Indeed.  The code to do that was hoisted up from DoURILoad to InternalLoad.

Er, to the _caller_ of InternalLoad.
BTW, whatever you decide to do, we would need in Gecko52 since that's what TB's next ESR will be based on. Branch day is next Monday, Nov. 14th, so the solution needs to get Aurora approval.
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #29)
> > but ShouldLoad isn't even called any more on the mailbox URL in question
> 
> Right, ShouldLoad is never called for system triggering principals.
> 
> > Where do we go from here? 
> 
> Good question.  Tanvi, do you have time to think about this, given that
> Christoph is out for the moment?  The basic issue is that we used to do
> content policy checks in docshell _before_ we decided what our triggering
> principal would be, and in particular passed a null (in the nullptr sense,
> not nullprincipal sense) principal to the content policy check.  This meant
> that in all sorts of "default load from chrome" cases we would invoke
> content policies.
> 
> With the changes in bug 1308889 we now compute the triggering principal
> earlier, and it has to be system to pass CheckLoadURI checks, but that means
> content policies get bypassed.
> 
> Do we want to do something to restore the old content policy behavior?  Or
> should we go ahead with the new content policy behavior?  In the latter
> case, mailnews will need some API to provide sandbox flags, or a CSP that
> can set sandbox flags, off some non-httpchannel interface...

I would like to preserve old content policy behavior - we should call content policies in all the places that we used to, and more.  We could bypass CheckLoadURI but still call Content Policies with a triggeringPrincipal ("requestingPrincipal") of System.  Arguably, we should still call it with nullptr in the places it used to be called with nullptr for the benefit of addon Content Policies.  But determining where those places were (the places that called Content Policy with nullptr requestingPrincipal) is difficult, and potentially impossible.

I'll look more into this this week.  Keeping my needinfo.

Jorg, how does this effect Tor Browser?  Do you create your own Content Policy?  For what purpose?
Flags: needinfo?(tanvi) → needinfo?(jorgk)
Flags: needinfo?(tanvi)
> we should call content policies in all the places that we used to, and more.

Content policy calls are not free.  We do not in fact want to do them any more than we absolutely have to....

> But determining where those places were (the places that called Content Policy with nullptr requestingPrincipal) is difficult, and potentially impossible.

For the specific case of docshell, we could just add yet another argument to InternalLoad and pass in the "principal to use for the content policy check".  It means we won't be able to push it down into the channel via AsyncOpen2 (or rather will now have _two_ content policy checks for every docshell load), but I don't see how we can change that while keeping the "do I open a new window?" logic sane.  Which is all very sad, of course...

If we do move toward always providing a triggering principal to LoadURI we would be back to square 1 here, though.

Maybe we should restore the old behavior for now and on branches, then come up with a new API that mailnews can use here, then remove the extra cruft for calling content policies...
Thanks for looking into it.

(In reply to Tanvi Vyas [:tanvi] from comment #36)
> Jorg, how does this effect Tor Browser?  Do you create your own Content
> Policy?  For what purpose?
Sorry, I know nothing about Tor. (I last installed it in Feb 2015 (no typo) to watch a YouTube video blocked in Germany, that's all ;-)) I'm a Thunderbird developer and interested in getting this issue fixed one way or another since we currently have test and noticeable functional failures on Trunk and Aurora.

We don't need a branch solution as Boris suggested. If you give us an interface to call within the next - say - two weeks, we're fine. It's not that a whole lot of e-mail gets sent with <noscript> tags and apparently JS is turned off as it should be, see comment #10.
Flags: needinfo?(jorgk)
(In reply to Jorg K (GMT+1) from comment #38)
> Thanks for looking into it.
> 
> (In reply to Tanvi Vyas [:tanvi] from comment #36)
> > Jorg, how does this effect Tor Browser?  Do you create your own Content
> > Policy?  For what purpose?
> Sorry, I know nothing about Tor. (I last installed it in Feb 2015 (no typo)
> to watch a YouTube video blocked in Germany, that's all ;-)) I'm a
> Thunderbird developer and interested in getting this issue fixed one way or
> another since we currently have test and noticeable functional failures on
> Trunk and Aurora.

Sorry for the confusion, saw you your comment 25 that said "TB" and "ESR" and assumed Tor Browser without reading the whole bug and seeing this is about Thunderbird ;)
Flags: needinfo?(tanvi)
Flags: needinfo?(tanvi)
See Also: → 1316087
Okay, read the whole bug and have a better sense of what is going on now.

I assumed[in 1] that the nsIContentPolicy behavior change would happen with bug https://bugzilla.mozilla.org/show_bug.cgi?id=1182569, when we started using LoadInfo member variables when calling Content Policies.  But with Bug 1308889, we actually changed the values we were passing into nsIContentPolicy (namely, requestingPrincipal) to match what we would pass into it when we use LoadInfo.

Boris, per comment 37, do you think its acceptable to call content policies twice for TYPE_DOCUMENT loads?  Once in InternalLoad and once with AsyncOpen2?  With potentially different requesting/triggeringPrincipal values?  If so, would my proposal in https://bugzilla.mozilla.org/show_bug.cgi?id=1316087#c4 work?

This way, all external Content Policies that depend on TYPE_DOCUMENT loads having a non-system triggeringPrincipal (i.e. the actual trigger or nullptr) will continue to work with the first call to ShouldLoad.  The second call to Content Policies/ShouldLoad would be skipped in the fallback to system case, and would otherwise be called with an actual trigger.  This isn't ideal, because sometimes the addon load would be rejected earlier and sometimes rejected later, without any real rhyme or reason from the addons perspective.

Alternatively, we can say that we don't want to worry about backwards compatibility.  And all addons (and thunderbird) need to adapt to the new behavior.  We could also do this in stages though; where right now we stick with two calls to Content Policy, and in the future we switch to just one with AsyncOpen2.



[1] (In reply to Tanvi Vyas [:tanvi] from comment #22)
> 
> What are the security implications of falling back to systemPrincipal for
> TYPE_DOCUMENT loads?  (For TYPE_SUBDOCUMENT loads we should add an assertion
> that requires triggeringPrincipal and doesn't allow a fallback to system.) 
> After the conversion in bug 1182569, loadInfo->triggeringPrincipal will be
> passed into NS_CheckContentLoadPolicy.  Before the conversion, if there is
> no triggeringPrincipal or referrer available, we would pass in a nullptr to
> NS_CheckContentLoadPolicy[2].  Hence, after the conversion, if there is no
> triggeringPrincipal or referrer available, we will pass in systemPrincipal
> to NS_CheckContentLoadPolicy instead of nullptr.
> do you think its acceptable to call content policies twice for TYPE_DOCUMENT loads?  

I think it's suboptimal, but I don't see any way to avoid it given our current general direction.  And it's not fatal, since document loads aren't _that_ common in the grand scheme of things.

> With potentially different requesting/triggeringPrincipal values?

The only case in which they would be different is if one is null and one is not, right?

Fwiw, passing a nullptr principal is really a bug in my opinion.  The nsIContentPolicy API documentation explicitly says that Gecko callers will _not_ do that...

> would my proposal in https://bugzilla.mozilla.org/show_bug.cgi?id=1316087#c4 work?

Something like that, wherein we propagate the fact that we had a nullptr principal in another argument would work as a quick-fix, sure.  See comment 37.

As I said in that bug, I don't think we should add docshell members here.

And again, it would just break again if/when we push the triggering principal determination out of docshell entirely and into its callers.

> Alternatively, we can say that we don't want to worry about backwards compatibility. 

In the long term, this is the right thing, imo.  The question is what we do for 52.

> And all addons (and thunderbird) need to adapt to the new behavior.

The only way for Thunderbird to adapt is for us to add some APIs that allow it to do so.  See the end of comment 29.  I don't have time to drive this.

> and in the future we switch to just one with AsyncOpen

I really don't see how you can possibly do that at any point in the future.
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #41)
> > do you think its acceptable to call content policies twice for TYPE_DOCUMENT loads?  
> 
> I think it's suboptimal, but I don't see any way to avoid it given our
> current general direction.  And it's not fatal, since document loads aren't
> _that_ common in the grand scheme of things.
> 
> > With potentially different requesting/triggeringPrincipal values?
> 
> The only case in which they would be different is if one is null and one is
> not, right?
> 
Yes.  In that case the first would be null and the second would be system.

> Fwiw, passing a nullptr principal is really a bug in my opinion.  The
> nsIContentPolicy API documentation explicitly says that Gecko callers will
> _not_ do that...
> 
Yet we have been doing it in nsDocShell.

> > would my proposal in https://bugzilla.mozilla.org/show_bug.cgi?id=1316087#c4 work?
> 
> Something like that, wherein we propagate the fact that we had a nullptr
> principal in another argument would work as a quick-fix, sure.  See comment
> 37.
> 
> As I said in that bug, I don't think we should add docshell members here.
> 
> And again, it would just break again if/when we push the triggering
> principal determination out of docshell entirely and into its callers.
> 
Okay, then another parameter to InternalLoad seems the only option to resolve the compatibility issues with addons and thunderbird.


> > Alternatively, we can say that we don't want to worry about backwards compatibility. 
> 
> In the long term, this is the right thing, imo.  The question is what we do
> for 52.
> 

Given Chris is out and I won't be able to make these changes (add an argument to InternalLoad that is used in the first call to Content Policies) or provide some sort of an API for mailnews (per comment 29), I think the only choice we have is to backout bug 1308889 from 52.  Then we can make the necessary changes for thunderbird and reland in 53 when Chris is back.

Boris, what do you think?
Flags: needinfo?(tanvi) → needinfo?(bzbarsky)
> Yet we have been doing it in nsDocShell.

Yes, because docshell is all broken like that. :(

> another parameter to InternalLoad seems the only option to
> resolve the compatibility issues with addons and thunderbird

If you want to keep the exact behavior Gecko used to have, then yes.

> I think the only choice we have is to backout bug 1308889 from 52

That would reintroduce the security bug that fixed; see bug 1308889 comment 24.  We'd need to back out more stuff; I'm not sure which stuff yet or what it would break if we backed it out.  Adding the extra argument is almost certainly a heck of a lot less work than figuring out what's safe to back out here.
Flags: needinfo?(bzbarsky)
When does Christoph come back? Will you be able to uplift any "forward" changes to Aurora?
> When does Christoph come back?

Dec 5. 

> Will you be able to uplift any "forward" changes to Aurora?

I think the extra arg would be eminently upliftable.
Boris, sadly it appears that bug 1308889 regressed bug 662907 thus causing bug 1318859.
See bug 1318859 comment #11.

In other words, messages in certain RSS feeds are currently broken.
Assignee: nobody → allstars.chh
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #41)> 
> > With potentially different requesting/triggeringPrincipal values?
> 
> The only case in which they would be different is if one is null and one is
> not, right?
> 
> Fwiw, passing a nullptr principal is really a bug in my opinion.  The
> nsIContentPolicy API documentation explicitly says that Gecko callers will
> _not_ do that...
> 
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #43)
> > another parameter to InternalLoad seems the only option to
> > resolve the compatibility issues with addons and thunderbird
> 
> If you want to keep the exact behavior Gecko used to have, then yes.
> 
Hi BZ
If we are going to add another parameter (a nsIPrincipal pass to CSP check) to InternalLoad, what value should it be?
Because your previous comment 41 said that we shouldn't pass nullptr...
but if it isn't nullptr nor SystemPrincipal, then what will it be?

Thanks
Tanvi's proposal would be to pass nullptr (in the cases when we used to pass nullptr).  In general it would be "whatever we used to pass to the content policy check".
Attached patch bug1316256.patch (obsolete) — — Splinter Review 
Hi BZ

This patch added a CSPPrincipal in InternalLoad, default to nullptr here, and it will be passed to CSP.

I've verified with make SOLO_TEST=content-policy/test-js-content-policy.js mozmill-one.

Not familiar how Thunderbird runs Try, so I didn't submit one.


(found that you disalbed r? and feedback?, so I change to ni? you)
Flags: needinfo?(bzbarsky)
(In reply to Yoshi Huang[:allstars.chh] from comment #50)
> I've verified with make SOLO_TEST=content-policy/test-js-content-policy.js
> mozmill-one.
That's all we need. If that passes, that's great.

> Not familiar how Thunderbird runs Try, so I didn't submit one.
They are tricky if you want to include an M-C patch. I can set on off for you. Perhaps Boris can comment on the patch first.

Thanks for getting this tackled ;-)
Passing nullptr in all cases is obviously wrong.  You should be passing the thing we used to pass, which was only null in a fairly small range of cases.

And this principal is being passed to content policy checks in general, not just CSP.

You also want comments in the IDL explaining why this argument exists and what callers are actually expected to pass for it (generally the same thing as aTriggeringPrincipal, yes?).
Flags: needinfo?(bzbarsky)
Attachment #8816086 - Flags: review-
Also, you need to actually use the new argument for something, right?
Yoshi, do you intend to keep working on this or should this be passed onto Christoph who is returning today? Thunderbird has been suffering breakage for almost a month now, so it would be good to get this fixed really soon.
Flags: needinfo?(allstars.chh)
Attached patch bug1316256.patch (JK v1). (obsolete) — — Splinter Review 
Yoshi, thanks for starting this off. This is a little urgent, so I took the liberty to carry on here. I believe I know what Boris had in mind.

Boris, can you please review. I'm using nullptr as principle for the content policy check in one case only where the triggering principal and the referrer are both null. I believe that's what you had in mind. Sorry, I know close to nothing about this area, so correct me if I'm wrong.

Our test
mozmake SOLO_TEST=content-policy/test-js-content-policy.js mozmill-one
passes with this patch.
Assignee: allstars.chh → jorgk
Attachment #8816086 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Flags: needinfo?(ckerschb)
Flags: needinfo?(bzbarsky)
Flags: needinfo?(allstars.chh)
Attached patch bug1316256.patch (JK v1b). (obsolete) — — Splinter Review 
Oops, this time without trailing spaces ;-)
Attachment #8816990 - Attachment is obsolete: true
(In reply to Jorg K (GMT+1) from comment #55)
> Our test
> mozmake SOLO_TEST=content-policy/test-js-content-policy.js mozmill-one
> passes with this patch.

Furthermore, <noscript> content in messages shows up again and the feed problem described in bug 1318859 is gone, too ;-)
Sorry I was in travel and didn't notice this is so urgent. :(
Yes, it's been broken for a month and impacting on Thunderbird tests (perma-red) and we even have a loss of functionality when looking at news feeds. Anyway, I picked up where you left off, I hope you don't mind. Thanks for starting, it became clear what needed to be done when looking at your patch.
Jorg, can you push to try?  On the surface, this looks good!  Christoph, can you review?  Neither you or Boris are accepting reviews right now, so needinfo for both of you.
Flags: needinfo?(ckerschb)
I can, but which try would you like? I usually work on the editor and only run mochitests. How about a full run but only on Linux?
Comment on attachment 8816992 [details] [diff] [review]
bug1316256.patch (JK v1b).

>+  nsCOMPtr<nsIPrincipal> cpcPrincipal = triggeringPrincipal;

How about naming this "principalForContentPolicyChecks"?

>+      cpcPrincipal = triggeringPrincipal;

This is in the !triggeringPrincipal && referrer case.  Why is this needed?  The old behavior would not have had this assignment, right?  I suspect you want to just remove it.

>+                         nsIPrincipal* aCPCPrincipal,

Likewise, aPrincipalForContentPolicyChecks.

>@@ -10046,16 +10056,17 @@ nsDocShell::InternalLoad(nsIURI* aURI,
>     if (NS_SUCCEEDED(rv) && targetDocShell) {
>+                                        aTriggeringPrincipal,

Should that be aPrincipalForContentPolicyChecks instead?  Did this bit use to be before we checked for null aTriggeringPrincipal or after?

>@@ -12507,16 +12518,17 @@ nsDocShell::LoadHistoryEntry(nsISHEntry*

This is a bit worrisome, because it will do the load differently than the original load...  Probably OK, since this whole principal-for-content-policy-checks business should be a temporary measure that we can remove at some point.

>+   * @param aCPCPrincipal        - Principal to be passed to content policy check.

Again, aPrincipalForContentPolicyChecks.  Please document that this should generally either match the triggering principal, but may be null in some cases when the triggering principal is system but we want to perform a content policy check anyway.

>+                              in nsIPrincipal aCPCPrincipal,

aPrincipalForContentPolicyChecks.

r=me with the above fixed and the things I asked to be checked checked.
Flags: needinfo?(bzbarsky)
Attachment #8816992 - Flags: review+
Attached patch bug1316256.patch (JK v1c). (obsolete) — — Splinter Review 
Boris, thank you very much for the quick review. I addressed the issues as well as I could, see below. Please check the patch again. I hope the interdiff will work.

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #63)
> >@@ -10046,16 +10056,17 @@ nsDocShell::InternalLoad(nsIURI* aURI,
> >     if (NS_SUCCEEDED(rv) && targetDocShell) {
> >+                                        aTriggeringPrincipal,
> Should that be aPrincipalForContentPolicyChecks instead?  Did this bit use
> to be before we checked for null aTriggeringPrincipal or after?
This is for the recursive call in nsDocShell::InternalLoad.

I don't understand what you're asking. Now I'm passing on aPrincipalForContentPolicyChecks. Are you referring to the changes in bug 1308889? Would you be able to check it yourself. Here are the changes in that function:
https://hg.mozilla.org/mozilla-central/rev/57d473c3ca22#l2.141
https://hg.mozilla.org/mozilla-central/rev/57d473c3ca22#l2.172
 
> >@@ -12507,16 +12518,17 @@ nsDocShell::LoadHistoryEntry(nsISHEntry*
> This is a bit worrisome, because it will do the load differently than the
> original load...  Probably OK, since this whole
> principal-for-content-policy-checks business should be a temporary measure
> that we can remove at some point.
I'm saving the triggering principle for the CPC, is that OK?
Attachment #8816992 - Attachment is obsolete: true
Flags: needinfo?(ckerschb) → needinfo?(bzbarsky)
Yep, interdiff works:
https://bugzilla.mozilla.org/attachment.cgi?oldid=8816992&action=interdiff&newid=8817138&headers=1

The changes to watch are:

Lines 12523-12529 and the hunk above:
triggeringPrincipal, --> principalForContentPolicyChecks,
in nsDocShell::LoadHistoryEntry() which is the bit you called "worrisome".

Lines 12523-12529:
triggeringPrincipal, --> principalForContentPolicyChecks,
in nsDocShell::InternalLoad which is the recursive call you wanted checked (and sadly I don't know how).
> I don't understand what you're asking.

I'm asking what this code used to do before bug 1308889.

The answer, looking at https://hg.mozilla.org/mozilla-central/file/5c1285f9911c/docshell/base/nsDocShell.cpp, is that we used to pass the original aTriggeringPrincipal, to the recursive call, before we modified it to be the system principal.  Which means the recursive call should be getting aPrincipalForContentPolicyChecks passed to it.  Thank you for changing that.

Looking at the same file, the principal we passed to CSP _was_ based on the referrer in some cases.  So you did in fact need the assignment in the "!triggeringPrincipal && referrer" case.  Please put that back.  This is why I asked a question about whether it was needed, instead of just telling you to remove it!  I sort of expected you to do the relevant due diligence instead of just pushing it all off on me.  :(

> I'm saving the triggering principle for the CPC, is that OK?

Won't really help anything; by this time the triggering principal is already clamped to system for all the cases in question.  The null-check there is just for cases when bogus history entries without a triggering principal are injected into the session history (e.g. by addons or session restore).  Please just take out the code you added to LoadHistoryEntry; it doesn't do anything in the cases where it might matter.
Flags: needinfo?(bzbarsky)
Comment on attachment 8817138 [details] [diff] [review]
bug1316256.patch (JK v1c).

r=me if the referrer bit is put back in.
Attachment #8817138 - Flags: review+
status-thunderbird51: unaffected → ---
status-thunderbird52: affected → ---
status-thunderbird53: affected → ---
tracking-thunderbird52: + → ---
Component: General → DOM: Security
Product: Thunderbird → Core
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #66)
> I sort of expected you to do the relevant due diligence
> instead of just pushing it all off on me.  :(
Thanks again for you're support. As I said, I know close to nothing about this, so my "due diligence" consisted in asking and making sure all changes get approved ;-)
Attached patch bug1316256.patch (JK v1d). (obsolete) — — Splinter Review 
Addressing issues and carrying forward Boris' r+.
Attachment #8817138 - Attachment is obsolete: true
Attachment #8817145 - Flags: review+
Keywords: checkin-needed
Keywords: checkin-needed
Backed out for failure in test_ext_webrequest_basic.html (this already failed in the Try push):

https://hg.mozilla.org/integration/mozilla-inbound/rev/d3bd1851509f1619ad5be143d1aff50d6a39a004

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=5ac1114601887748a9ed081909a4c915ff1cee36
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=40351053&repo=mozilla-inbound

[task 2016-12-07T12:27:02.703788Z] 12:27:02     INFO - TEST-PASS | toolkit/components/extensions/test/mochitest/test_ext_webrequest_basic.html | correct ip for http://mochi.test:8888/tests/toolkit/components/extensions/test/mochitest/file_WebRequest_page3.html?trigger=a - Expected: 127.0.0.1, Actual: 127.0.0.1 
[task 2016-12-07T12:27:02.705548Z] 12:27:02     INFO - SpawnTask.js | Leaving test test_webRequest_tabId
[task 2016-12-07T12:27:02.708469Z] 12:27:02     INFO - SpawnTask.js | Entering test test_webRequest_tabId_browser
[task 2016-12-07T12:27:02.710111Z] 12:27:02     INFO - Extension loaded
[task 2016-12-07T12:27:02.711905Z] 12:27:02     INFO - onBeforeRequest 18 data:application/vnd.mozilla.xul+xml;charset=utf-8,%3C?xml%20version=%221.0%22?%3E%0A%20%20%3Cwindow%20id=%22documentElement%22/%3E
[task 2016-12-07T12:27:02.714118Z] 12:27:02     INFO - Buffered messages finished
[task 2016-12-07T12:27:02.722629Z] 12:27:02     INFO - TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/mochitest/test_ext_webrequest_basic.html | unexpected request application/vnd.mozilla.xul+xml;charset=utf-8,%3C 
[task 2016-12-07T12:27:02.724839Z] 12:27:02     INFO -     getExpected@moz-extension://45367c01-9f40-4862-9b60-f6ecac2878c5/%7B9e89bd3d-3486-4683-afef-75798ec84bf1%7D.js:53:7
[task 2016-12-07T12:27:02.727072Z] 12:27:02     INFO -     getListener/<@moz-extension://45367c01-9f40-4862-9b60-f6ecac2878c5/%7B9e89bd3d-3486-4683-afef-75798ec84bf1%7D.js:146:22
[task 2016-12-07T12:27:02.729021Z] 12:27:02     INFO -     
[task 2016-12-07T12:27:02.731415Z] 12:27:02     INFO - onCompleted 18 data:application/vnd.mozilla.xul+xml;charset=utf-8,%3C?xml%20version=%221.0%22?%3E%0A%20%20%3Cwindow%20id=%22documentElement%22/%3E
[task 2016-12-07T12:27:02.734780Z] 12:27:02     INFO - Not taking screenshot here: see the one that was previously logged
[task 2016-12-07T12:27:02.740321Z] 12:27:02     INFO - TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/mochitest/test_ext_webrequest_basic.html | unexpected request application/vnd.mozilla.xul+xml;charset=utf-8,%3C 
[task 2016-12-07T12:27:02.744981Z] 12:27:02     INFO -     getExpected@moz-extension://45367c01-9f40-4862-9b60-f6ecac2878c5/%7B9e89bd3d-3486-4683-afef-75798ec84bf1%7D.js:53:7
[task 2016-12-07T12:27:02.751095Z] 12:27:02     INFO -     getListener/<@moz-extension://45367c01-9f40-4862-9b60-f6ecac2878c5/%7B9e89bd3d-3486-4683-afef-75798ec84bf1%7D.js:146:22
[task 2016-12-07T12:27:02.752626Z] 12:27:02     INFO -
Flags: needinfo?(jorgk)
Sorry, yes, I did look at the try results at
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b03c01dc39b3e5674cc312947eb5c8a5ba4daa7a

and saw some Mochitest failures "5" and "10" reading:
===
TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/mochitest/test_ext_webrequest_basic.html | unexpected request application/vnd.mozilla.xul+xml;charset=utf-8,%3C

1317619 Intermittent toolkit/components/extensions/test/mochitest/test_ext_webrequest_basic.html | resource type is correct - Expected: main_frame, Actual: other
===

Sorry, I thought they were intermittent, but I didn't look closely enough and the suggestion doesn't match the actual error. My mistake.

I wouldn't know what's going in here, maybe it has got something to do with the original change in bug 1308889:
https://hg.mozilla.org/mozilla-central/rev/57d473c3ca22#l12.2

So I have to ask Boris again for his help or we need to wait for Christoph.
Flags: needinfo?(jorgk)
Flags: needinfo?(ckerschb)
Flags: needinfo?(bzbarsky)
This test got changed quite a bit after bug 1308889 so it's possible that the new version would have failed before that change too.

Looks like it's seeing a "data:application/vnd.mozilla.xul+xml;charset=utf-8,<" (probably with more stuff after it, but something truncated the message?).  See http://searchfox.org/mozilla-central/source/toolkit/components/extensions/test/mochitest/head_webrequest.js#52-66

At a guess, this load could be coming from http://searchfox.org/mozilla-central/source/toolkit/components/extensions/ext-backgroundPage.js#20 or so?

Shane, do you know offhand whether a load of that URL is expected during this test, and whether it would be picked up by what this test is doing, in general?  And also whether the same would have been true before the big test refactor?

Jorg, one thing you should try is simply backing out bug 1308889 and seeing whether this test fails then.  If it does not, then we're not quite matching the pre-1308889 behavior.  If it does, then we probably just need to change the test expectations or some other aspect of the test (e.g. to avoid that data: load).
Flags: needinfo?(mixedpuppy)
Flags: needinfo?(jorgk)
Flags: needinfo?(bzbarsky)
Building with bug 1308889 (rev 57d473c3ca22) backed out now (78 conflicts, one to be manually resolved, I hope I got that right). I'll post the result here, if it builds at all ;-)
Flags: needinfo?(jorgk)
I'll have to look at what is happening, I'll try to get to that this afternoon.
OK, I asked on IRC and was told to |mach clobber python| (bug 1321468).

toolkit/components/extensions/test/mochitest/test_ext_webrequest_basic.html is a "monster" test and in the end I got

1680 INFO Passed:  672
1681 INFO Failed:  2
1682 INFO Todo:    0

1688 INFO TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/mochitest/test-oop-extensions/test_ext_webrequest_basic.html | unexpected request application/vnd.mozilla.xul+xml;charset=utf-8,%3C
    getExpected@moz-extension://b2cf39e5-9638-43cd-b005-8d7b6e0c7fd6/%7B33c58cfe-5dc1-4737-a35a-80450bc489d4%7D.js:53:7
    getListener/<@moz-extension://b2cf39e5-9638-43cd-b005-8d7b6e0c7fd6/%7B33c58cfe-5dc1-4737-a35a-80450bc489d4%7D.js:146:22

1689 INFO TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/mochitest/test-oop-extensions/test_ext_webrequest_basic.html | unexpected request application/vnd.mozilla.xul+xml;charset=utf-8,%3C
    getExpected@moz-extension://b2cf39e5-9638-43cd-b005-8d7b6e0c7fd6/%7B33c58cfe-5dc1-4737-a35a-80450bc489d4%7D.js:53:7
    getListener/<@moz-extension://b2cf39e5-9638-43cd-b005-8d7b6e0c7fd6/%7B33c58cfe-5dc1-4737-a35a-80450bc489d4%7D.js:146:22

1690 INFO TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/mochitest/test_ext_webrequest_basic.html | unexpected request application/vnd.mozilla.xul+xml;charset=utf-8,%3C
    getExpected@moz-extension://b1662859-b073-4de7-a226-ec37d81779b4/%7B56a3b936-91b5-4981-85eb-1c5a6163e252%7D.js:53:7
    getListener/<@moz-extension://b1662859-b073-4de7-a226-ec37d81779b4/%7B56a3b936-91b5-4981-85eb-1c5a6163e252%7D.js:146:22

1691 INFO TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/mochitest/test_ext_webrequest_basic.html | unexpected request application/vnd.mozilla.xul+xml;charset=utf-8,%3C
    getExpected@moz-extension://b1662859-b073-4de7-a226-ec37d81779b4/%7B56a3b936-91b5-4981-85eb-1c5a6163e252%7D.js:53:7
    getListener/<@moz-extension://b1662859-b073-4de7-a226-ec37d81779b4/%7B56a3b936-91b5-4981-85eb-1c5a6163e252%7D.js:146:22

So Boris was spot on. These tests wouldn't pass with the pre-bug 1308889 state of things.
OK.  That makes it pretty likely we just need to update the test expectations; I'm hoping Shane can figure out pretty quickly what the right way to do that is.
So it looks like a system principal frame is bypassing our checks and is leaking through webrequest into the addon.  This shouldn't be happening (it's bad).  The url hitting the test is the background frame created for background scripts, the top level which is has system principal (per file location in comment 73)

Would the change in this patch affect this logic here at all?

https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/addons/WebRequest.jsm#627

That value is later used to avoid calling into the webextension listener, here:

https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/ext-webRequest.js#25
The patch here should not affect either of those, I would think.  It should only affect what nsIContentPolicy shouldLoad implementations see.  (I'm not saying it _doesn't_ affect them, I'm saying it _shouldn't_.)

So it would affect whether http://searchfox.org/mozilla-central/rev/a8b5f53e7df90df655a0982e94087ee83290c22e/toolkit/modules/addons/WebRequestContent.js#81 gets called, I would think: without this patch it won't be, but with this patch it will.
I looked into what is happening [without the patch here] and I am not able to create a situation where I see this data url go through webrequest or webrequestcontent.  I haven't had time to get around to building with the patch.  

@Kris, do you see anything obvious here?  See comment 79
Flags: needinfo?(mixedpuppy) → needinfo?(kmaglione+bmo)
To be clear, what this patch will do is make us call shouldLoad for this data: URL.  Without this patch we do not call shouldLoad for it.
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #73)
> Looks like it's seeing a
> "data:application/vnd.mozilla.xul+xml;charset=utf-8,<" (probably with more
> stuff after it, but something truncated the message?).  See
> http://searchfox.org/mozilla-central/source/toolkit/components/extensions/
> test/mochitest/head_webrequest.js#52-66
> 
> At a guess, this load could be coming from
> http://searchfox.org/mozilla-central/source/toolkit/components/extensions/
> ext-backgroundPage.js#20 or so?

Hm. That's the URL we load into the windowless browser, to create the document that holds our background page <browser>. That windowless browser is a typeChrome docshell, so we actually shouldn't be calling WebExtension content policy listeners for it at all.
Flags: needinfo?(kmaglione+bmo)
This seems to be getting difficult. Am I reading correctly that the content policy checks we're bringing back to the pre-bug 1308889 state are actually not desired?
Well, as Kris mentioned above, we certainly do not want the typeChrome docshell urls passing through to webextensions, so that would mean some kind of additional fix, either in the patch or in WebRequest.  I'm not really clear about pre-bug 1308889 behavior myself.  The test refactoring I did for webrequests makes the tests stricter, they are meant to fail if unexpected requests make it through to the listeners.

So, if we're absolutely certain that this patch produces the right behavior, we probably need to make some additional check in WebRequestContent.js.
(In reply to Shane Caraveo (:mixedpuppy) from comment #85)
> I'm not really clear about pre-bug 1308889 behavior myself.
I can only speak for the Thunderbird side. As you can see in comment #26, in pre-bug 1308889 state ShouldLoad is called for our mailbox URL and the triggering principal is null (as in nullptr). Debug in comment #28 shows the current state where the triggering principal has become system and ShouldLoad is therefore not called.

This the patch here, the triggering principal is also system, but a different null/nullptr principal is passed to the content policy check.

> So, if we're absolutely certain that this patch produces the right behavior,
> we probably need to make some additional check in WebRequestContent.js.
Only Boris or Christoph can decide that.
Jorg, I haven't read all of the contents of that bug. From what I understand nsIContentPolicy is not consulted because the principal got changed from a nullptr to a SystemPrincipal and hence nsIContentPolicy implementations which would be important in your case don't get called, right?

I briefly looked at the problem this morning. The reason we bail out of contentPolicy checks is because of performance reasons. In particular we don't want to consult contentPolicies for chrome code at Firefox startup.

Consulting contentPolicies for TYPE_DOCUMENT would be acceptable (assuming it's TYPE_DOCUMENT for what you need). Unfortunately I don't have time to compile, debug and look at the problem in detail, but I'll post some code underneath which hopefully does the trick. Any chance you could apply the patch and do some debugging to see if that fixes the problem? Boris would be fine with accepting and landing such a patch.

--- a/dom/base/nsContentPolicyUtils.h
+++ b/dom/base/nsContentPolicyUtils.h
@@ -176,17 +176,17 @@ NS_CP_ContentTypeName(uint32_t contentTy
       if (!secMan) {                                                          \
           secMan = do_GetService(NS_SCRIPTSECURITYMANAGER_CONTRACTID);        \
       }                                                                       \
       if (secMan) {                                                           \
           bool isSystem;                                                      \
           nsresult rv = secMan->IsSystemPrincipal(originPrincipal,            \
                                                   &isSystem);                 \
           NS_ENSURE_SUCCESS(rv, rv);                                          \
-          if (isSystem) {                                                     \
+          if (isSystem && contentType != nsIContentPolicy::TYPE_DOCUMENT)     \
               *decision = nsIContentPolicy::ACCEPT;                           \
               nsCOMPtr<nsINode> n = do_QueryInterface(context);               \
               if (!n) {                                                       \
                   nsCOMPtr<nsPIDOMWindowOuter> win = do_QueryInterface(context);\
                   n = win ? win->GetExtantDoc() : nullptr;                    \
               }                                                               \
               if (n) {                                                        \
                   nsIDocument* d = n->OwnerDoc();                             \
Flags: needinfo?(ckerschb) → needinfo?(jorgk)
Any patch that fixes this bug for Thunderbird by ensuring that we make the content policy checks will cause exactly the same problem for the webrequest code.

It sounds like the timeline here is:

1)  We used to do the content policy check, and the webrequest tests did not care about that.
2)  We stopped doing the content policy check, breaking Thunderbird.
3)  The webrequest tests were changed to be stricter and fail if we do the check.
4)  We now want to go back to doing the check, to unbreak Thunderbird.

So yes, we're absolutely sure the patch produces the "right behavior" in terms of whether the content policy check is done.  Christoph's suggestion in comment 87 is just a simpler approach to getting to that goal.
  
Long-term, we will want to fix bug 1322618 (after fixing bug 1322617) and then we can remove the change we're making in this bug...

But in the meantime, it sounds like we need to make changes to WebRequestContent.js to filter out whatever it is it's trying to filter out.  I'm happy to help figure out what those changes should be if someone can describe to me what the filtering criteria are meant to be.  Blindly needinfoing Kris, because I can't tell which of him and Shane knows this stuff better.  ;)
Flags: needinfo?(kmaglione+bmo)
I think we probably just need to filter out shouldLoad calls in WebRequestContent.js where the requestPrincipal is the system principal. We already do that for the HTTP observers in the parent process, in WebRequest.jsm, so it's a simple enough change.

It would be nice to also ignore calls for typeChrome docshells, but it might be better to just stay consistent with what we do for HTTP observers there too.
Flags: needinfo?(kmaglione+bmo)
If we do that, then we definitely want the comment 87 approach.  The approach from attachment 8817145 [details] [diff] [review] would have the requestPrincipal be null, not system.
Yeah, I can't think of a better option at this point. We definitely don't want those listeners called for a data: URL that winds up inheriting the system prinicipal, so if shouldLoad is going to be called for them at all, we need to know when it's being loaded by the system principal.
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #87)
> Jorg, I haven't read all of the contents of that bug. From what I understand
> nsIContentPolicy is not consulted because the principal got changed from a
> nullptr to a SystemPrincipal and hence nsIContentPolicy implementations
> which would be important in your case don't get called, right?

Right. I will try your patch for TB and FF and report back here. Thanks!
Flags: needinfo?(jorgk)
Here is Christoph's patch. It fixes the TB issue but as predicted by Boris in comment #88 (quote) " ... cause[s] exactly the same problem for the webrequest code".

I'll spare you copying the test results here again, they look the same as in comment #77.

So I'll have to leave it to Boris, Christoph, Kris and Shane to figure out how to fix those tests.

Thanks to all for your help so far.
This approach still wont work as far as I can see.  The principal being passed is the triggering principal.  If we reject based on that being a system principal, we will reject any top level load (e.g. content loading in a tab) initiated from chrome (eg. awesomebar).  We also get null for node (so unable to check typeChrome), and null for requestOrigin, so I have no other data to try and correlate with.

We handle detecting top level loads in the request listener by relying on loadingPrincipal, inheritPrincipal *and* triggeringPrincipal, per the code I pointed to in comment 79.

The tests are not broken, there is nothing to fix there.  They in fact operated as expected by catching an unexpected url passing through that should not have, in this case one that really cannot slip through.  The tests prior to my refactor would not have caught this issue since catching this problem is dependent on the use of two extensions being used in the test.

So...we need a bit more analysis from someone with deeper knowledge of nsContentPolicy than I have, and whether the fix actually should be in thunderbird rather than nsContentPolicy.

bz, if you're around work week, you should be able to find me usually close to the firefox room (kohala 4).
Flags: needinfo?(bzbarsky)
I guess we can just special case URLs that inherit principals for document loads, and ignore the requesting principal otherwise.
Shane,

I can't make sense of your comment so far, because I'm not sure what the goal of your content policy implementation is, and I can't find any documentation for it in WebRequestContent.js...  It's not clear to me why you would need to "reject" (in the nsIContentPolicy sense) anything at all, for example.

I would be happy to meet with your or Kris or both later this afternoon.  I have meetings until 3:30 or so in Kohala 4.  If you can explain to me what the goal of this code is, that would be quite useful.

> We handle detecting top level loads in the request listener by relying on
> loadingPrincipal, inheritPrincipal *and* triggeringPrincipal

Right, and all you have in content policy is triggering principal.

> The tests prior to my refactor would not have caught this issue

It's still not clear to me what the issue is, and the only reason the tests passed when landed is because there was a Gecko regression that broke backwards compat.  We're just trying to back out the compat breakage part.  So what would you have done if we had _not_ regressed Gecko here?

> whether the fix actually should be in thunderbird rather than nsContentPolicy

That's bug 1322618, but that's obviously quite inappropriate as something to backport to 52, which is where we need this fixed.  So we do in fact need to fix the behavior regression in nsContentPolicy for now, restoring the behavior we have in 51, while we sort out how we want to structure all this going forward.
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #96)
> I would be happy to meet with your or Kris or both later this afternoon.  I
> have meetings until 3:30 or so in Kohala 4.  If you can explain to me what
> the goal of this code is, that would be quite useful.

The content policy is used to notify extension listeners of non-HTTP content
loads, and to allow them to block those loads.

> It's still not clear to me what the issue is, and the only reason the tests
> passed when landed is because there was a Gecko regression that broke
> backwards compat.  We're just trying to back out the compat breakage part.
> So what would you have done if we had _not_ regressed Gecko here?

The restrictions against monitoring requests triggered by the system
principal, and the tests that show the problem, were added while this was
regressed, so I suppose we just would have had to deal with it sooner.
Attached patch patch with webrequest fix (obsolete) — — Splinter Review 
Here's a patch with a fix to WebRequestContent.  Asking kmag for review on the webext change.
Attachment #8817744 - Flags: review?(kmaglione+bmo)
Can you validate the fix on your end?
Flags: needinfo?(jorgk)
Shane, thank you very much for your patch. As I said in comment #93, Christoph's change in nsContentPolicyUtils.h fixes the problem for Thunderbird. Your hunk makes the test_ext_webrequest_basic.html test pass. The result is:

0 INFO TEST-START | Shutdown
1 INFO Passed:  1344
2 INFO Failed:  0
3 INFO Todo:    0
4 INFO Mode:    e10s
5 INFO SimpleTest FINISHED
Buffered messages finished
SUITE-END | took 443s

Compared to comment #77 this seems to have run twice (1344 = 2 * 672).
Flags: needinfo?(jorgk)
Also see:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2c615f4be237c97c893b8e97c0875b9afbccfaa0
(pushed in two patches since I already had Christoph's patch applied and didn't want to cause a big local rebuild due to changing a .h file)
Attachment #8817145 - Attachment is obsolete: true
Attachment #8817590 - Attachment is obsolete: true
Kris, gentle ping for the review.

Thunderbird is broken for more than a month now. Can we please get this sorted out soon. We also need uplift to Aurora here, so the longer we wait, the harder it will be to convince the decision makers to approve the uplift.
Flags: needinfo?(kmaglione+bmo)
Comment on attachment 8817744 [details] [diff] [review]
patch with webrequest fix

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

r=me for the WebRequestContent changes, but I'm not qualified to review the nsContentPolicyUtils changes.

::: toolkit/modules/addons/WebRequestContent.js
@@ +81,5 @@
>    shouldLoad(policyType, contentLocation, requestOrigin,
>               node, mimeTypeGuess, extra, requestPrincipal) {
> +    if (requestPrincipal &&
> +        Services.scriptSecurityManager.isSystemPrincipal(requestPrincipal)) {
> +          return Ci.nsIContentPolicy.ACCEPT;

Nit: Extra indentation.
Attachment #8817744 - Flags: review?(ckerschb)
Flags: needinfo?(kmaglione+bmo)
Comment on attachment 8817744 [details] [diff] [review]
patch with webrequest fix

Thanks. Christoph wrote that one line, so he can't approve it ;-)
Asking Boris (who's not taking reviews).

Also, who's going to land this with a commit message? Boris, would you be able to push this to inbound for us.
Flags: needinfo?(bzbarsky)
Attachment #8817744 - Flags: review?(kmaglione+bmo)
Attachment #8817744 - Flags: review?(ckerschb)
And fix the indentation:
+    if (requestPrincipal &&
+        Services.scriptSecurityManager.isSystemPrincipal(requestPrincipal)) {
+      return Ci.nsIContentPolicy.ACCEPT;
+    }

Boris, or if you simply approve the nsContentPolicyUtils.h bit, I can fix the patch and get it landed with r=bz,kmag.
... and author Christoph Kerschbaumer and Shane Caraveo ... or split it again into two patches, we already have one in attachment 8817590 [details] [diff] [review].
Sorry for the noise. Maybe two patches would be better since I'm not sure that the WebRequestContent changes need uplift to Aurora. Do you know where/when item 3) in comment #88 landed?
(In reply to Jorg K (GMT+1) from comment #107)
> Sorry for the noise. Maybe two patches would be better since I'm not sure
> that the WebRequestContent changes need uplift to Aurora. Do you know
> where/when item 3) in comment #88 landed?

Landed in 52, so that will need uplift if you want to fix 52.
Comment on attachment 8817744 [details] [diff] [review]
patch with webrequest fix

r=me on the nsContentPolicyUtils bits.
Flags: needinfo?(bzbarsky)
Attachment #8817744 - Flags: review+
Comment on attachment 8817590 [details] [diff] [review]
1316256-alternative.patch (CK v1).

Carrying forward Boris' r+ from comment #109.

I' am going to split this in two patches, that way it's easier to attribute this to the correct author and reviewer.
Attachment #8817590 - Attachment is obsolete: false
Attachment #8817590 - Flags: review+
Attached patch webrequest fix — — Splinter Review 
Carrying forward Kris' r+ from comment #103.
Attachment #8817744 - Attachment is obsolete: true
Attachment #8819013 - Flags: review+
Aleth, can you please push both patches to inbound. I hope they still apply.

Thanks to all involved.
Flags: needinfo?(aleth)
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ba8dff4a242a7ddaf6da214e5fbde8fb95e9b12
Bug 1316256 - Always call content policy check for TYPE_CONTENT. r=bz

https://hg.mozilla.org/integration/mozilla-inbound/rev/c2576212405e8574f39b34923ef5aaf256ad074c
Bug 1316256 - WebRequestContent part: return nsIContentPolicy.ACCEPT in shouldLoad for request principal system. r=kmag
Flags: needinfo?(aleth)
https://hg.mozilla.org/mozilla-central/rev/7ba8dff4a242
https://hg.mozilla.org/mozilla-central/rev/c2576212405e
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment on attachment 8817590 [details] [diff] [review]
1316256-alternative.patch (CK v1).

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1308889 (which landed on mozilla52)
[User impact if declined]: Thunderbird not functional, since mail display and feed/news display broken.
[Is this code covered by automated tests?]: Yes, we had to adapt them.
[Has the fix been verified in Nightly?]: On a Thunderbird Daily build.
[Needs manual test from QE? If yes, steps to reproduce]: I can supply tests for Thunderbird if required.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]:
This returns the content policy checks to the pre-bug 1308889 state. The changes have been extensively discussed with Boris who is the Docshell owner.
[String changes made/needed]: None.

There will be follow-up work in bug 1322617 and bug 1322618 to untangle this situation, also see comment #88. For now, we need this uplifted to mozilla52.
Attachment #8817590 - Flags: approval-mozilla-aurora?
Comment on attachment 8819013 [details] [diff] [review]
webrequest fix

Approval Request Comment
See previous comment.

This patch adapts the test.

Boris, do you have anything to add here, I filled in the form to the best of my knowledge.
Flags: needinfo?(bzbarsky)
Attachment #8819013 - Flags: approval-mozilla-aurora?
I think the change is somewhat risky, but less risky than any of the other options we've considered.  Nothing to add past that.
Flags: needinfo?(bzbarsky)
Comment on attachment 8817590 [details] [diff] [review]
1316256-alternative.patch (CK v1).

call content policy check for more content, fix thunderbird regression in aurora52

I got confused here for a while because the commit message says TYPE_CONTENT where it looks like it meant TYPE_DOCUMENT.
Attachment #8817590 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8819013 [details] [diff] [review]
webrequest fix

webextensions followup fix for content policy change, take in aurora52
Attachment #8819013 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to Julien Cristau [:jcristau] from comment #119)
> I got confused here for a while because the commit message says TYPE_CONTENT
> where it looks like it meant TYPE_DOCUMENT.
My apologies, I was handling patches for other authors and I sadly got the commit message wrong.

Thanks for taking it! That saves me a lot of work since otherwise I would have had to carry around these patches on a special branch to use for our Thunderbird ESR 52.
We still seem to get this error, don't we?
(See try-comm-central, for example.).
I meant to include this piece of local log from full DEBUG version (plus a few extra dump) of TB:

TEST-START | /NREF-COMM-CENTRAL/comm-central/mail/test/mozmill/content-policy/test-js-content-policy.js | test_jsContentPolicy
++DOMWINDOW == 116 (0x55d1159a9f20) [pid = 15365] [serial = 260] [outer = 0x55d11827c420]
(debug) Creating buffered output stream to mboxFile=<</NREF-COMM-CENTRAL/objdir-tb3/_tests/mozmill/mozmillprofile/Mail/Local Folders/jsContentPolicy>> in nsMsgBrkMBoxStore::GetNewMsgOutputStream in nsMsgBrkMBoxStore.cpp;
(debug) creating a buffered outFileStream in nsMsgLocalMailFolder::AddMessageBatch().
++DOMWINDOW == 117 (0x55d11b3e4d10) pid = 15365] [serial = 261] [outer = 0x55d11827c420]
[15365] WARNING: NS_ENSURE_TRUE(standardURL) failed: file /NREF-COMM-CENTRAL/comm-central/mozilla/caps/nsPrincipal.cpp, line 185
[15365] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file /NREF-COMM-CENTRAL/comm-central/mozilla/caps/BasePrincipal.cpp, line 378
[15365] WARNING: 'NS_FAILED(rv)', file /NREF-COMM-CENTRAL/comm-central/mozilla/dom/workers/ServiceWorkerManager.cpp, line 1868
[15365] WARNING: Failed to retarget HTML data delivery to the parser thread.: file /NREF-COMM-CENTRAL/comm-central/mozilla/parser/html/nsHtml5StreamParser.cpp, line 970
TEST-UNEXPECTED-FAIL | /NREF-COMM-CENTRAL/comm-central/mail/test/mozmill/content-policy/test-js-content-policy.js | test-js-content-policy.js::test_jsContentPolicy

Question or rather a mystery:
Re the following error:
[15365] WARNING: NS_ENSURE_TRUE(standardURL) failed: file /NREF-COMM-CENTRAL/comm-central/mozilla/caps/nsPrincipal.cpp, line 185

  177	  // If we reached this branch, we can only create an origin if we have a
  178	  // nsIStandardURL.  So, we query to a nsIStandardURL, and fail if we aren't
  179	  // an instance of an nsIStandardURL nsIStandardURLs have the good property
  180	  // of escaping the '^' character in their specs, which means that we can be
  181	  // sure that the caret character (which is reserved for delimiting the end
  182	  // of the spec, and the beginning of the origin attributes) is not present
  183	  // in the origin string
  184	  nsCOMPtr<nsIStandardURL> standardURL = do_QueryInterface(origin);
  185	  NS_ENSURE_TRUE(standardURL, NS_ERROR_FAILURE);

The comment is so opaque in the context of TB, it is hard to understand why this fails.

Anyway, this is one of the few failures in local and treeherder mozmill test now.
> We still seem to get this error, don't we?

It got regressed again.  See bug 1329288, filed based on bug 1328847 comment 25.
(In reply to ISHIKAWA, Chiaki from comment #123)
> We still seem to get this error, don't we?
I publish information about current test failures in the C-C treeherder info box, currently it reads:
Two X and Z failures - Bug 1328847
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: