Bug 1273138 added a filter preventing WebRequest from viewing loads from system principals. This caused a regression preventing WebRequest being able to see top level content loads. It's overly aggressive since it uses either triggeringPrincipal or loadingPrincipal. The top level content loads do not have a loadingPrincipal and should not be filtered. The current check is: let {isSystemPrincipal} = Services.scriptSecurityManager; data.isSystemPrincipal = (isSystemPrincipal(loadInfo.triggeringPrincipal) || isSystemPrincipal(loadInfo.loadingPrincipal)); but should be changed to: data.isSystemPrincipal = loadInfo.loadingPrincipal && (isSystemPrincipal(loadInfo.triggeringPrincipal) || isSystemPrincipal(loadInfo.loadingPrincipal));

It's unfortunate that we don't already have tests for this. But given the inscrutability of the webRequest tests, I'm not really surprised.

Note that in addition to the new test, test_chrome_ext_webrequest_background_events.html fails all over if data.isSystemPrincipal is wrong. After reading through nsILoadInfo.idl several times, I beleive I have a correct check for system principal, and the new test works with top level browser loads.

Comment on attachment 8813447 [details] Bug 1318800 fix systemPrincipal checks so top level tab loads work with webrequests, https://reviewboard.mozilla.org/r/94838/#review95296 ::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_basic.html:241 (Diff revision 1) > > +add_task(function* test_webRequest_tabId_browser() { > + function background(url) { > + let tabId; > + browser.test.onMessage.addListener((msg, expected) => { > + browser.tabs.remove(tabId).then(() => { Please use async/await. ::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_basic.html:246 (Diff revision 1) > + browser.tabs.remove(tabId).then(() => { > + browser.test.sendMessage("done"); > + }); > + }); > + > + browser.tabs.create({url}).then(tab => { tabId = tab.id; }); Please use async/await. ::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_basic.html:268 (Diff revision 1) > + // expecting origin == undefined > + extension.sendMessage("set-expected", {expect}); > + yield extension.awaitMessage("continue"); > + > + // open a tab from a system principal > + yield tabExt.startup(); I don't think we should rely on the tabs API using the system principal as the triggering principal. This is probably better done as a browser mochitest... But I guess that can be done in a follow-up. ::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_basic.html:274 (Diff revision 1) > + > + yield extension.awaitMessage("done"); > + tabExt.sendMessage("done"); > + yield tabExt.awaitMessage("done"); > + yield tabExt.unload(); > +}); It would be nice to test whether we see requests for sub-frames of system principal documents, or window.open calls from likewise. But, again, follow-up is fine. ::: toolkit/modules/addons/WebRequest.jsm:632 (Diff revision 1) > + if (loadInfo.loadingPrincipal) { > + data.isSystemPrincipal = isSystemPrincipal(loadInfo.loadingPrincipal); > + } else if (loadInfo.principalToInherit) { > + data.isSystemPrincipal = isSystemPrincipal(loadInfo.principalToInherit); > + } else { > + data.isSystemPrincipal = isSystemPrincipal(loadInfo.triggeringPrincipal); > + } isSystemPrincipal(loadInfo.loadingPrincipal || loadInfo.principalToInherit || loadInfo.triggeringPrincipal);

Comment on attachment 8813447 [details] Bug 1318800 fix systemPrincipal checks so top level tab loads work with webrequests, https://reviewboard.mozilla.org/r/94838/#review96110 ::: browser/components/extensions/test/browser/browser_ext_webRequest.js:115 (Diff revision 2) > + let openedWindow = Services.ww.openWindow( > + null, Services.prefs.getCharPref("browser.chromeURL"), "_blank", > + "chrome,dialog=no,all", null); > + > + yield TestUtils.topicObserved("browser-delayed-startup-finished", > + subject => subject == openedWindow); yield BrowserTestUtils.openNewBrowserWindow();

The last push to review fixes a cache issue when running with other tests, but has a problem with browser_ext_webNavigation_urlbar_transitions.js, which is preventing test_newWindow in browser_ext_webRequest.js from completing. Still looking into why.

Figured it out, favicon is being loaded during some tests prior to the new test. I then don't get the request as I was expecting in the test. It's not important, so I added an ignore capability and am ignoring favicon if I happen to get it (which is the case when running the test solo).

Pushed by mixedpuppy@gmail.com: https://hg.mozilla.org/integration/autoland/rev/09b480229ec2 fix systemPrincipal checks so top level tab loads work with webrequests, r=kmag

Pushed by kwierso@gmail.com: https://hg.mozilla.org/mozilla-central/rev/2176b72e7e2f fix systemPrincipal checks so top level tab loads work with webrequests, r=kmag

Approval Request Comment [Feature/Bug causing the regression]: webrequest [User impact if declined]: top level page loads are not observable [Is this code covered by automated tests?]: yes [Has the fix been verified in Nightly?]: yes [Needs manual test from QE? If yes, steps to reproduce]: no Aurora version of patch only differs in modification to browser.ini for the test file. [List of other uplifts needed for the feature/fix]: [Is the change risky?]: [Why is the change risky/not risky?]: [String changes made/needed]:

(In reply to Shane Caraveo (:mixedpuppy) from comment #18) > Created attachment 8815883 [details] [diff] [review] > 1318800 aurora version of patch Somehow I missed the following: > [List of other uplifts needed for the feature/fix]: none > [Is the change risky?]: no > [Why is the change risky/not risky?]: it's covered pretty extensively in tests now > [String changes made/needed]: none

Comment on attachment 8815883 [details] [diff] [review] 1318800 aurora version of patch webextensions request handling fix for aurora52