STR: - Go to about:addons with e10s enabled - Ctrl+U AR: Doesn't work.

Started happening beginning-mid September I think.

(In reply to Tim Nguyen [:ntim] from comment #0) > AR: > Doesn't work. To be more precise, it opens a blank tab.

Pushlog: https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=8045d8fcbfe5474276037faccb5863746830326b&tochange=9efdda8b4a18 Regressed by: Bug 1201535

Bug likely exists back to 42, but e10s is disabled for 42 beta, so I think we only need to consider 43 and 44.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8b5bb9fa8370

Bug 1213693 - Repair view source tab for parent process only URLs. r=mconley

Comment on attachment 8672712 [details] MozReview Request: Bug 1213693 - Repair view source tab for parent process only URLs. r=mconley https://reviewboard.mozilla.org/r/21775/#review19669 ::: browser/base/content/browser.js:2346 (Diff revision 1) > + // Some URLs cannot be loaded in a remote browser. View source in tab > + // expects the new view source browser's remoteness to match that of the > + // original URL, so disable remoteness if necessary for this URL. > + let contentProcess = Ci.nsIXULRuntime.PROCESS_TYPE_CONTENT > + let forceNotRemote = > + gMultiProcessBrowser && > + !E10SUtils.canLoadURIInProcess(args.URL, contentProcess); Hrm. But there's already logic in ViewSourceBrowser.jsm (specifically in the loadViewSource method) to check if the browser we're looking at is remote, and if so, to set the remoteness on the newly opened tab as appropriate. Can we not make use of that? Perhaps by ensuring we're passing in the browser that we want to view the source from in the args that we pass to viewSourceInBrowser?

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #7) > Comment on attachment 8672712 [details] > MozReview Request: Bug 1213693 - Repair view source tab for parent process > only URLs. r=mconley > > https://reviewboard.mozilla.org/r/21775/#review19669 > > ::: browser/base/content/browser.js:2346 > (Diff revision 1) > > + // Some URLs cannot be loaded in a remote browser. View source in tab > > + // expects the new view source browser's remoteness to match that of the > > + // original URL, so disable remoteness if necessary for this URL. > > + let contentProcess = Ci.nsIXULRuntime.PROCESS_TYPE_CONTENT > > + let forceNotRemote = > > + gMultiProcessBrowser && > > + !E10SUtils.canLoadURIInProcess(args.URL, contentProcess); > > Hrm. But there's already logic in ViewSourceBrowser.jsm (specifically in the > loadViewSource method) to check if the browser we're looking at is remote, > and if so, to set the remoteness on the newly opened tab as appropriate. > > Can we not make use of that? Perhaps by ensuring we're passing in the > browser that we want to view the source from in the args that we pass to > viewSourceInBrowser? Currently the view source code only updates the remoteness of <browser>s in view source windows[1]. This case seems simple enough, since the view source window is managing it's own <browser>. For the case of view source in tab, it's currently defined to throw an error if the remoteness does not match.[2] Since the browser window is effectively "loaning" the <browser> for use as a view source tab, the view source code does not currently attempt to flip remoteness and re-insert the <browser>, as I guessed there might be other browser tab state involved that could get confused. Was I being too conservative, and view source should instead flip remoteness of any <browser>? The reason this issue did not occur until after bug 1201535 is because we used to have the browser window create a new view-source:<URL> tab instead of about:blank as we do now. This means the tabbrowser code was setting the remoteness for us based on the URL to be shown (I believe by calling canLoadURIInProcess itself). We had to switch away from this in bug 1201535 to avoid sending unnecessary GETs on the URL, and that meant that now no one was flipping the remoteness anymore. [1]: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/viewsource/content/viewSource.js#668 [2]: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/viewsource/ViewSourceBrowser.jsm#231

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #8) > (In reply to Mike Conley (:mconley) - Needinfo me! from comment #7) > > Comment on attachment 8672712 [details] > > MozReview Request: Bug 1213693 - Repair view source tab for parent process > > only URLs. r=mconley > > > > https://reviewboard.mozilla.org/r/21775/#review19669 > > > > ::: browser/base/content/browser.js:2346 > > (Diff revision 1) > > > + // Some URLs cannot be loaded in a remote browser. View source in tab > > > + // expects the new view source browser's remoteness to match that of the > > > + // original URL, so disable remoteness if necessary for this URL. > > > + let contentProcess = Ci.nsIXULRuntime.PROCESS_TYPE_CONTENT > > > + let forceNotRemote = > > > + gMultiProcessBrowser && > > > + !E10SUtils.canLoadURIInProcess(args.URL, contentProcess); > > > > Hrm. But there's already logic in ViewSourceBrowser.jsm (specifically in the > > loadViewSource method) to check if the browser we're looking at is remote, > > and if so, to set the remoteness on the newly opened tab as appropriate. > > > > Can we not make use of that? Perhaps by ensuring we're passing in the > > browser that we want to view the source from in the args that we pass to > > viewSourceInBrowser? > > Currently the view source code only updates the remoteness of <browser>s in > view source windows[1]. This case seems simple enough, since the view > source window is managing it's own <browser>. > > For the case of view source in tab, it's currently defined to throw an error > if the remoteness does not match.[2] Since the browser window is > effectively "loaning" the <browser> for use as a view source tab, the view > source code does not currently attempt to flip remoteness and re-insert the > <browser>, as I guessed there might be other browser tab state involved that > could get confused. > > Was I being too conservative, and view source should instead flip remoteness > of any <browser>? I think we can get away with that - at least, unless I'm not considering some edge cases. I would imagine in the majority of cases, the browser we're flipping remoteness on has just opened as a new tab, so there's really no state to need to worry about. The only case I can think of needing to manage is the manually entered view-source:// URI case, but I believe we support opening the view-source URI in the content process, so whatever the remoteness of the browser that the user started with should still work fine if view-source:// was put in... So to sum, it sounds to me as if the only browsers we need to worry about were just opened. If that's the case, flipping their remoteness should be fine. Or am I skipping any cases?

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #9) > So to sum, it sounds to me as if the only browsers we need to worry about > were just opened. If that's the case, flipping their remoteness should be > fine. > > Or am I skipping any cases? I believe that's the only case. I'll give this approach a try then.

This variation allows view source to flip tab remoteness, but it leaves me with a broken tab instead. I tested this using view source on about:addons. It seems like it could work if I instead called `gBrowser.updateBrowserRemoteness` on tabbrowser, but the view source module doesn't normally call into tabbrowser directly, so it seems odd to start now.

Comment on attachment 8673980 [details] [diff] [review] Attempt allowing view source to flip tab remoteness You're right - the logic to do the remoteness change for tab <xul:browser>'s is contained within tabbrowser.xml's updateBrowserRemoteness, and is significantly more complex than what we're doing here. I also agree that calling into gBrowser seems clumsy and out of place within ViewSourceBrowser. Considering the small number of pages that this is going to apply to (basically, all of our blacklisted about: pages), your original workaround is probably okay, with some caveats. Let me re-review it.

Comment on attachment 8672712 [details] MozReview Request: Bug 1213693 - Repair view source tab for parent process only URLs. r=mconley https://reviewboard.mozilla.org/r/21775/#review19811 ::: browser/base/content/browser.js:2346 (Diff revision 1) > + // Some URLs cannot be loaded in a remote browser. View source in tab > + // expects the new view source browser's remoteness to match that of the > + // original URL, so disable remoteness if necessary for this URL. > + let contentProcess = Ci.nsIXULRuntime.PROCESS_TYPE_CONTENT > + let forceNotRemote = > + gMultiProcessBrowser && > + !E10SUtils.canLoadURIInProcess(args.URL, contentProcess); This comment should probably make it clear that the "some URLs" in question is a very small set, and all internal (chrome://, some about:, etc). ::: browser/base/content/browser.js:2361 (Diff revision 1) > + forceNotRemote Nit - please add trailing comma

https://reviewboard.mozilla.org/r/21775/#review19811 > This comment should probably make it clear that the "some URLs" in question is a very small set, and all internal (chrome://, some about:, etc). Okay, I've updated the comment. > Nit - please add trailing comma Fixed.

Comment on attachment 8672712 [details] MozReview Request: Bug 1213693 - Repair view source tab for parent process only URLs. r=mconley Bug 1213693 - Repair view source tab for parent process only URLs. r=mconley

Comment on attachment 8672712 [details] MozReview Request: Bug 1213693 - Repair view source tab for parent process only URLs. r=mconley Approval Request Comment [Feature/regressing bug #]: Regressed by bug 1201535 [User impact if declined]: Opening view source of a few internal pages, like about:addons, that are not e10s ready would fail [Describe test coverage new/current, TreeHerder]: On m-c, no additional tests [Risks and why]: Seems low, should only affect internal pages. [String/UUID change made/needed]: None

Comment on attachment 8672712 [details] MozReview Request: Bug 1213693 - Repair view source tab for parent process only URLs. r=mconley Low-risk fix for recent regression. OK to uplift to aurora.