STR: 1: Open http://yt-dash-mse-test.commondatastorage.googleapis.com/unit-tests/2017.html?&test_type=conformance-test&timestamp=1487670424161 2: Right click and select "View Page Source" 3: Click on "js/harness/main-20161221170552.js" link 4: Press the Back button. 5: Boom! Assertion failure: triggeringPrincipal (need a valid triggeringPrincipal to load from history), at /Users/jyavenard/Work/Mozilla/mozilla-central/docshell/base/nsDocShell.cpp:12633 #01: nsDocShell::LoadHistoryEntry(nsISHEntry*, unsigned int)[/Users/jyavenard/Work/Mozilla/mozilla-central/obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x683a694] #02: nsDocShell::LoadURI(nsIURI*, nsIDocShellLoadInfo*, unsigned int, bool)[/Users/jyavenard/Work/Mozilla/mozilla-central/obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x6838e39] #03: non-virtual thunk to nsDocShell::LoadURI(nsIURI*, nsIDocShellLoadInfo*, unsigned int, bool)[/Users/jyavenard/Work/Mozilla/mozilla-central/obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x683fe8e] #04: nsSHistory::InitiateLoad(nsISHEntry*, nsIDocShell*, long)[/Users/jyavenard/Work/Mozilla/mozilla-central/obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x68a8fb0] #05: nsSHistory::LoadDifferingEntries(nsISHEntry*, nsISHEntry*, nsIDocShell*, long, bool&)[/Users/jyavenard/Work/Mozilla/mozilla-central/obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x68ab714] #06: nsSHistory::LoadEntry(int, long, unsigned int)[/Users/jyavenard/Work/Mozilla/mozilla-central/obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x68a85e0] #07: nsSHistory::GoBack()[/Users/jyavenard/Work/Mozilla/mozilla-central/obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x68a7cb7] #08: non-virtual thunk to nsSHistory::GoBack()[/Users/jyavenard/Work/Mozilla/mozilla-central/obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x68a8669] #09: nsDocShell::GoBack()[/Users/jyavenard/Work/Mozilla/mozilla-central/obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x6850ca1] #10: non-virtual thunk to nsDocShell::GoBack()[/Users/jyavenard/Work/Mozilla/mozilla-central/obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/XUL +0x6850cec] Killed: 9 Process exited with status 137 logout

Hi Christoph, the assertion was added by you in bug 1307736. Reading your comment bug 1307736 comment 66, I am assigning this bug to you for now. Do feel free to reset if you don't have time soon-ish, thanks!

I couldn't reproduce this issue to provide the tracking flags on the latest Firefox Nightly and on the latest Firefox release on Windows 10 x 64 and Mac OS X 10.10. Can you please provide with more info about the Firefox version you used and the OS?

NI :jya for comment 2. Thanks!

(In reply to Hani Yacoub from comment #2) > I couldn't reproduce this issue to provide the tracking flags on the latest > Firefox Nightly and on the latest Firefox release on Windows 10 x 64 and Mac > OS X 10.10. > you need a debug build

Could you please set the tracking flags so we could know if it's a regression?

I have already set the bug that caused this regression

Gijs, as discussed over IRC I tried to create a simplified model of the STRs from comment 0 and package them into an automated test. The important part is the 'going back to view-source' which loads the history entry that would trigger the assertion to fail in case there is no valid triggeringPrincipal. I think that's what we want to test in the end.

Gijs, it seems Olli is not accepting review requests at the moment. Any chance you could also review the actual patch? It seems that we were not setting a triggeringPrincipal when creating an shEntry for view-source loads. I did an DXR search for 'entry->SetURI' as well as for 'entry.SetURI' to make sure we are not missing any other cases like this one. It seems that we have updated all other cases except the two within this changeset.

Comment on attachment 8842108 [details] [diff] [review] bug_1341589_set_triggeringprincipal_view_source.patch Review of attachment 8842108 [details] [diff] [review]: ----------------------------------------------------------------- rs=me for the viewSource-content.js change, but I don't feel I can review the docshell change. For one, because I'm not a peer, but for another because it looks like we'll just be using system principal for all history loads, which feels like a bad idea?

Comment on attachment 8842106 [details] [diff] [review] bug_1341589_test_triggeringprincipal_view_source.patch Review of attachment 8842106 [details] [diff] [review]: ----------------------------------------------------------------- This isn't terrible, but I have some comments. :-) ::: docshell/test/browser/browser_history_triggeringprincipal_viewsource.js @@ +1,3 @@ > +"use strict"; > + > +const PATH = "http://www.example.com/browser/docshell/test/browser/"; Please use: const TEST_PATH = getRootDirectory(gTestPath).replace("chrome://mochitests/content", "http://example.com"); which will continue to work if someone moves the test or changes the harness. @@ +10,5 @@ > + yield ContentTask.spawn(gBrowser.selectedBrowser, HTML_URI, HTML_URI => { > + content.document.location = HTML_URI; > + }); > + yield loadPromise; > + is(content.document.location.href, HTML_URI, "loading HTML succeeded"); Generally, modifying the main tab that's loaded is a bad idea because it affects later tests. So instead we tend to create new tabs when writing browser tests. Here, I would just use: yield BrowserTestUtils.withNewTab(HTML_URI, function*(browser) { // in here the tab has already loaded. This will also remove the tab when your inner task has finished. // the argument (browser) is the browser for the new tab it opens (so you can replace your use of gBrowser.selectedBrowser with it). }); @@ +22,5 @@ > + let viewSourceItem = contextMenu.getElementsByAttribute("id", "context-viewsource")[0]; > + viewSourceItem.click(); > + contextMenu.hidePopup(); > + let tab = yield tabPromise; > + is(content.document.location.href, VIEW_SRC_URI, "loading view-source of html succeeded"); This is using a CPOW. Check gBrowser.selectedBrowser.currentURI.spec instead (here and below). @@ +25,5 @@ > + let tab = yield tabPromise; > + is(content.document.location.href, VIEW_SRC_URI, "loading view-source of html succeeded"); > + > + info ("load the html file another time"); > + let loadPromise1 = BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser, false, HTML_URI); So at this point, are you loading the file into the view-source tab? You can be more explicit about this by passing tab.linkedBrowser instead. @@ +28,5 @@ > + info ("load the html file another time"); > + let loadPromise1 = BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser, false, HTML_URI); > + yield ContentTask.spawn(gBrowser.selectedBrowser, HTML_URI, HTML_URI => { > + content.document.location = HTML_URI; > + }); I think this can probably just use BrowserTestUtils.loadURI. @@ +37,5 @@ > + let contextMenu2 = document.getElementById("contentAreaContextMenu"); > + let popupPromise2 = BrowserTestUtils.waitForEvent(contextMenu2, "popupshown"); > + BrowserTestUtils.synthesizeMouseAtCenter("body", { type: "contextmenu", button: 2 }, gBrowser.selectedBrowser); > + yield popupPromise2; > + let loadPromise2 = BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser, false, VIEW_SRC_URI); You could just reassign to loadPromise without suffixing '2' - just make sure to omit the duplicate 'let' as you no longer need to declare the variable. @@ +39,5 @@ > + BrowserTestUtils.synthesizeMouseAtCenter("body", { type: "contextmenu", button: 2 }, gBrowser.selectedBrowser); > + yield popupPromise2; > + let loadPromise2 = BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser, false, VIEW_SRC_URI); > + let backItem = contextMenu2.getElementsByAttribute("id", "context-back")[0]; > + backItem.click(); I think to go back, you can just use gBrowser.selectedBrowser (or tab.linkedBrowser) .goBack() without affecting whether the test tests this particular path? In which case, that's a lot simpler. :-) ::: docshell/test/browser/file_history_triggeringprincipal_viewsource.html @@ +1,4 @@ > +<html> > +<head> <meta charset="utf-8"> </head> > + <body> > + just a dummy html file making sure view-source history entries have a valid triggeringPrincipal What we've done elsewhere is just call this dummy_page.html or empty_page.html so other tests that need "something" to load into a tab can reuse it, rather than having lots of test-specific files literring the tree. You may also be able to use a data: URI for this particular testcase.

(In reply to :Gijs from comment #9) > rs=me for the viewSource-content.js change, but I don't feel I can review > the docshell change. For one, because I'm not a peer, but for another > because it looks like we'll just be using system principal for all history > loads, which feels like a bad idea? I looked at this in a little more detail again and I think you are right. In fact, we should only change viewSource-content.js and don't touch docshell at all for this bug. I know you already reviewed the viewSource changes but I wanted to make sure you agree that we should not touch docshell in which case you could r+ the patch or otherwise let me know your concerns. thanks!

Comment on attachment 8842802 [details] [diff] [review] bug_1341589_set_triggeringprincipal_view_source.patch Review of attachment 8842802 [details] [diff] [review]: ----------------------------------------------------------------- Happy to r+ this, but is this sufficient to fix the bug? Is there a remaining bug now for history loads that open view-source? If so we could potentially make that a followup, I guess.

Oh, and you may also need to fix the 'view selection source' case. Can you check that manually?

(In reply to :Gijs from comment #10) > Comment on attachment 8842106 [details] [diff] [review] > bug_1341589_test_triggeringprincipal_view_source.patch Gijs, thanks for the detailed response. Really learned something here and I hope my coding of browser tests will not take ages in the future :-) Anyway, I incorporated all of your suggestions besides two things: *) Using BrowserTestUtils.loadURI did not work, hence I kept ContentTask.spawn(...). I don't know what the exact issue is, but it simply did not work they way I wanted and I didn't want to invest more time to figure out the exact problem :-( *) I kept the 'right-click -> go back' part of the test. First because I already had it and it might be useful for other people writing a similar test to copy that part of the test. If you insist however, I would invest more time to figure out and change the two parts of your suggestions I haven't incorporated. Thanks!

(In reply to :Gijs from comment #13) > Oh, and you may also need to fix the 'view selection source' case. Can you > check that manually? Good call. I just checked and 'view selection source' works properly even without the patch from this bug applied.

(In reply to :Gijs from comment #12) > Happy to r+ this, but is this sufficient to fix the bug? Is there a > remaining bug now for history loads that open view-source? If so we could > potentially make that a followup, I guess. I think that's it for view-source. I will manually test a little more to make sure everything works fine. Worst case I have to file a new bug.

Comment on attachment 8842805 [details] [diff] [review] bug_1341589_test_triggeringprincipal_view_source.patch Review of attachment 8842805 [details] [diff] [review]: ----------------------------------------------------------------- ::: docshell/test/browser/browser_history_triggeringprincipal_viewsource.js @@ +37,5 @@ > + popupPromise = BrowserTestUtils.waitForEvent(backCtxtMenu, "popupshown"); > + BrowserTestUtils.synthesizeMouseAtCenter("body", { type: "contextmenu", button: 2 }, aBrowser); > + yield popupPromise; > + loadPromise = BrowserTestUtils.browserLoaded(tab.linkedBrowser, false, VIEW_SRC_URI); > + let backItem = backCtxtMenu.getElementsByAttribute("id", "context-back")[0]; Looks like this can just be document.getElementById("context-back"); ?

Pushed by mozilla@christophkerschbaumer.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/855aa2d7063c Set triggeringPrincipal on history entry for view-source loads. r=gijs https://hg.mozilla.org/integration/mozilla-inbound/rev/263c21fb7308 Test TriggeringPrincipal on history entry for view-source loads. r=gijs

https://hg.mozilla.org/mozilla-central/rev/855aa2d7063c https://hg.mozilla.org/mozilla-central/rev/263c21fb7308

Please request Aurora approval on this when you get a chance.

Comment on attachment 8842802 [details] [diff] [review] bug_1341589_set_triggeringprincipal_view_source.patch Approval Request Comment This bug does not cause immediate end user facing problems because within Bug 1337622 we added code to revert to the old behavior before Bug 1307736 landed. Nevertheless, uplifting this bug would have history entries to have the desired/accurate triggeringPrincipal. The assertion we hit here is only in debug builds. In release builds it works fine using reverted semantics. [Feature/Bug causing the regression]: Bug 1307736 - Assert history loads pass a valid triggeringPrincipal for docshell loads [User impact if declined]: See Approval request [Is this code covered by automated tests?]: Yes, an automated test landed on mc with this bug. [Has the fix been verified in Nightly?]: I haven't tested manually - but we have an automated test. [Needs manual test from QE? If yes, steps to reproduce]: No [List of other uplifts needed for the feature/fix]: --- [Is the change risky?]: No, not risky. [Why is the change risky/not risky?]: We are only setting an explicit triggeringPrincipal to history entries instead of using a built-in fallback mechanism. [String changes made/needed]: No

Comment on attachment 8842805 [details] [diff] [review] bug_1341589_test_triggeringprincipal_view_source.patch Approval Request Comment See comment 21.

Comment on attachment 8842802 [details] [diff] [review] bug_1341589_set_triggeringprincipal_view_source.patch Regression from 53, let's take these 2 patches for beta 2.

Comment on attachment 8842805 [details] [diff] [review] bug_1341589_test_triggeringprincipal_view_source.patch Thanks for the test!

https://hg.mozilla.org/releases/mozilla-beta/rev/e6da0317da15 https://hg.mozilla.org/releases/mozilla-beta/rev/9f2bf992c9a8