[Tracking Requested - why for this release]: [Tracking Requested - why for this release]: [Tracking Requested - why for this release]: See bug 1194764 comment 15. This is a problem in beta, but I doubt it's possible to fix it fast enough to ship Firefox 41 with a working view-source. So I think we should just disable view-source-in-tab for 41, and maybe 42, until we get this sorted out...

Let's make sure we agree on the scope of the problem first. In general, view source tab, e10s on *does* work with ex. a POST correctly: 1. BrowserViewSource[1] passes the outerWindowID. 2. ViewSourceBrowser.jsm sends the message[2] ViewSource:LoadSource with URL and outerWindowID. 3. We get the page descriptor from the window ID[3]. Here's a test case: 1. Go to http://www.pangloss.com/seidel/Poem/ 2. Click "Groove" to send a POST and get a random poem 3. Open View Source Also, the general frame source case also works: 1. Go to http://www.htmq.com/html/sample/frame.htm 2. Right click in some frame, View Frame Source I believe the only problem case for view source tab is viewing the frame source of srcdoc frames. Do you agree? [1]: https://dxr.mozilla.org/mozilla-central/rev/a6786bf8d71d4cf40c3d40e06d8e3c9866863475/browser/base/content/browser.js#2400 [2]: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/viewsource/ViewSourceBrowser.jsm?offset=0#198 [3]: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/viewsource/content/viewSource-content.js#231

> 1. Go to http://www.pangloss.com/seidel/Poem/ > 2. Click "Groove" to send a POST and get a random poem > 3. Open View Source Exciting. This works in beta and aurora, but fails on nightly, as far as I can tell.

(In reply to Boris Zbarsky [:bz] from comment #2) > > 1. Go to http://www.pangloss.com/seidel/Poem/ > > 2. Click "Groove" to send a POST and get a random poem > > 3. Open View Source > > Exciting. This works in beta and aurora, but fails on nightly, as far as I > can tell. Hmm, I was testing in Nightly myself... I tested again, this time with a clean profile, and seems to still be working in Nightly. Can you check on your end again with a clean profile in Nightly?

Yes, I tested with a clean profile. 2015-09-03 nightly, load http://www.pangloss.com/seidel/Poem/ then click "groove" then Command-U. Does not show the right source. Except... I guess this only happens if I click "groove" before the spinner stops on the http://www.pangloss.com/seidel/Poem/ url. OK, so that's a separate issue, I guess, though still pretty fishy. So I compared what docshell sees in the pangloss case and the srcdoc case. In the pangloss case, I see two loads both happening in the same docshell: 1) A load of view-source:http://www.pangloss.com/seidel/Poem/poem.cgi coming from the addTab call that loadOneTab makes when called from BrowserViewSourceOfDocument. This does NOT have the right page descriptor. 2) A load of view-source:http://www.pangloss.com/seidel/Poem/poem.cgi coming from loadSource when viewSource-content.js gets the message. In the srcdoc case, I see four loads like so: 1) view-source:about:srcdoc with no page descriptor, just like in the pangloss case. 2) about:neterror triggered by that load. 3) A second call to LoadURIWithOptions via loadURIWithFlags via addTab (why?) 4) A second about:neterror There is never a load for the real thing with a page descriptor.

Ah, so what happens is that the loadURIWithOptions call throws NS_ERROR_MALFORMED_URI because you're not allowed to do that. This exception is caught in _loadURIWithFlags and another attempt is made to do the load after updating remoteness. This throws too, which propagates up and aborts things. So the real problem here is that initial load that view-source-in-tab performs on just the URI, and the user-observable correctness problem is in fact probably restricted to srcdoc iframes.

Though of course the underlying problem of doing the extra GET when viewing source of a POST result is present too; it's just not obviously visible to the _user_.

Tracked for 41+. Just fyi, we are a week away from gtb 41 RC.

I believe it may take some time to devise a proper fix for this issue, and it may not land in time for 41. I would consider this to be a lower severity issue, as I don't believe viewing source of srcdoc frames is something many people will attempt. Jeff, do you think it's okay to keep view source tabs enabled in 41 with this regression?

I think the web-server-facing issue with the extra GET on viewing source of a POST document (this is NOT limited to viewing frame source, afaict) is also something we shouldn't ship, personally...

I'm fine with disabling this for now in release - I disagree that it is a large issue, srcdoc is not well-used at this point but the current user experience if someone *does* run across it is not great. Ryan it seems like you have a reasonable plan for a fix - we should see about resourcing after q3.

Comment on attachment 8658428 [details] [diff] [review] Disable view source tabs on Release / Beta Thank you! Sorry for all the trouble, but I really do think this is the right call for one more release...

Comment on attachment 8658428 [details] [diff] [review] Disable view source tabs on Release / Beta Approval Request Comment [Feature/regressing bug #]: View source in tab [User impact if declined]: This patch will disable the view source in tab feature for beta / release, until regressions like this are resolved. [Describe test coverage new/current, TreeHerder]: No additional tests, only a pref change. [Risks and why]: Low, only changes a pref value. [String/UUID change made/needed]: None.

Try for Beta: https://treeherder.mozilla.org/#/jobs?repo=try&revision=783bc3f2c4f9

Flagging Ritu directly since there's not much time left. Is it okay to uplift a patch to disable this feature in beta?

Boris, Ryan: I agree in general that if a feature is mostly broken, it is best not to ship it until it is ready. Is that the case here? It seems to me that a large portion of our end-users would not even try this scenario as it is not a common scenario. Is that an incorrect assumption? Also, if they do hit this issue, they can workaround it by disabling view source tabs by setting "view_source.tab"=false. Given this, do we still need to disable this feature (turn the pref off)?

Ritu, which scenario are you talking about? There are two related problems here: 1) Any time you view source, no matter how or of what, we issue an extra request to the server and then ignore the response. This can have weird unintended effects for servers with non-idempotent GET handlers (think "delete" links) and whatnot. Think you view source and it actually changes state on the server. 2) In the particular case of about:srcdoc iframes, trying to do #1 fails completely and as a result the source can't be viewed at all. I think just #1 on its own, even ignoring the about:srcdoc interaction, is a bad enough problem that we should fix it before shipping.

(In reply to Boris Zbarsky [:bz] from comment #19) > Ritu, which scenario are you talking about? > > There are two related problems here: > > 1) Any time you view source, no matter how or of what, we issue an extra > request to the server and then ignore the response. This can have weird > unintended effects for servers with non-idempotent GET handlers (think > "delete" links) and whatnot. Think you view source and it actually changes > state on the server. > > 2) In the particular case of about:srcdoc iframes, trying to do #1 fails > completely and as a result the source can't be viewed at all. > > I think just #1 on its own, even ignoring the about:srcdoc interaction, is a > bad enough problem that we should fix it before shipping. Thank you for providing the additional context. I was under the impression that this bug is just about not being able to view source of srcdoc frames (unable to read/parse/display content) and can be worked around by turning the view source tab pref. If under the hood the current implementation of view source tab can mess up the state on the server at times, then this problem is more severe and needs to be re-designed/implemented. I will approve the beta patch uplift that disables this feature.

Comment on attachment 8658428 [details] [diff] [review] Disable view source tabs on Release / Beta The additional GET request that is being sent when viewing source is worrisome as it could mess up the state of the server. Preff'ing view source in tab off by default in 41 makes sense. Let's uplift to Beta41.

Sylvestre, to make a call on whether this needs to be pref'd off for 42 as well. Given that 42 is a week away from entering Beta, it makes sense but we could keep it on while trying to resolve these issues too.

(In reply to Ritu Kothari (:ritu) from comment #22) > Sylvestre, to make a call on whether this needs to be pref'd off for 42 as > well. Given that 42 is a week away from entering Beta, it makes sense but we > could keep it on while trying to resolve these issues too. The current patch disables the feature in Release / Beta channels, so this would happen for 42 once it moves to Beta unless further changes are made.

Marking branch-patch-needed, as I need to also set the testing prefs to enable tabs. I'll land this on fx-team first once the tree opens again.

(In reply to Boris Zbarsky [:bz] from comment #19) ... > 1) Any time you view source, no matter how or of what, we issue an extra > request to the server and then ignore the response. This can have weird > unintended effects for servers with non-idempotent GET handlers (think > "delete" links) and whatnot. Think you view source and it actually changes > state on the server. Now, let's be clear and identify this as incredibly bad behaviour on the part of the server - a really well-known anti-pattern. > 2) In the particular case of about:srcdoc iframes, trying to do #1 fails > completely and as a result the source can't be viewed at all. Again, while this is actually true, use of srcdoc is likely quite low. I agree with bz that we should pref off for 41 in release, but I don't want to overstate the end-user impact. This feature has been live in dev edition for several cycles and we have no complaints from 100s of thousands of web developers of the srcdoc issue.

> Now, let's be clear and identify this as incredibly bad behaviour on the part of the > server Indeed. This is a well-known nono which servers still do every so often. :(

Jryans landed this followup to beta: https://hg.mozilla.org/releases/mozilla-beta/rev/904c9cae1be1

Comment on attachment 8658428 [details] [diff] [review] Disable view source tabs on Release / Beta OK, let's do that in 42 too then.

Pushed to aurora: remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/8ebca8b445df remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/b15f772f84a2

Ritu, can you remove view source tabs from the 41 relnotes? We can redo for a future release once it's back on again. It was originally relnoted in bug 1067325, so that bug's relnote flag should be reset I suppose.

This was removed from the 41 release notes.

Bug 1201535 - Open view source tabs with about:blank. r=bz Loading a view source tab as "about:blank" gets us the fresh tab we need without making redundant requests. The view source module will retrieve the actual data as needed.

Bug 1201535 - Mark srcdoc as safe for loading in child. r=bz When showing view source of a srcdoc frame in a tab, we load the frame content via a page descriptor and use the URI "view-source:about:srcdoc". Since we are creating a browser tab, the docshell's `InternalLoad` triggers the browser's `shouldLoadURI` machinery that checks if we are in the correct process for the given URI. This machinery will unwrap "view-source:" to "about:srcdoc" and check if such an about page can be loaded in the child. Marking "about:srcdoc" as safe for the child to load allows the view source content of the frame to be displayed as expected.

Bug 1201535 - Test view source of srcdoc frames. r=mconley

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

Comment on attachment 8665109 [details] MozReview Request: Bug 1201535 - Test view source of srcdoc frames. r=mconley https://reviewboard.mozilla.org/r/20143/#review18083 Looks good! Two minor suggestions. ::: toolkit/components/viewsource/test/browser/browser_srcdoc.js:5 (Diff revision 1) > +let frameSource = `<a href="about:mozilla">good</a>`; > +let source = `<html><iframe srcdoc='${frameSource}' id="f"></iframe></html>`; Maybe make these consts? ::: toolkit/components/viewsource/test/browser/browser_srcdoc.js:10 (Diff revision 1) > + let tab = yield BrowserTestUtils.openNewForegroundTab(gBrowser, uri); Alternatively, you can use BrowserTestUtils.withNewTab, which takes your test function as an argument, and automatically closes the tab once the test function has exited.

Comment on attachment 8665107 [details] MozReview Request: Bug 1201535 - Open view source tabs with about:blank. r=bz https://reviewboard.mozilla.org/r/20139/#review18213 r=me

Comment on attachment 8665108 [details] MozReview Request: Bug 1201535 - Mark srcdoc as safe for loading in child. r=bz https://reviewboard.mozilla.org/r/20141/#review18215 r=me

https://reviewboard.mozilla.org/r/20143/#review18083 > Maybe make these consts? Okay, done! > Alternatively, you can use BrowserTestUtils.withNewTab, which takes your test function as an argument, and automatically closes the tab once the test function has exited. Sounds good, I've changed to that.

Comment on attachment 8665107 [details] MozReview Request: Bug 1201535 - Open view source tabs with about:blank. r=bz Bug 1201535 - Open view source tabs with about:blank. r=bz Loading a view source tab as "about:blank" gets us the fresh tab we need without making redundant requests. The view source module will retrieve the actual data as needed.

Comment on attachment 8665108 [details] MozReview Request: Bug 1201535 - Mark srcdoc as safe for loading in child. r=bz Bug 1201535 - Mark srcdoc as safe for loading in child. r=bz When showing view source of a srcdoc frame in a tab, we load the frame content via a page descriptor and use the URI "view-source:about:srcdoc". Since we are creating a browser tab, the docshell's `InternalLoad` triggers the browser's `shouldLoadURI` machinery that checks if we are in the correct process for the given URI. This machinery will unwrap "view-source:" to "about:srcdoc" and check if such an about page can be loaded in the child. Marking "about:srcdoc" as safe for the child to load allows the view source content of the frame to be displayed as expected.

Comment on attachment 8665109 [details] MozReview Request: Bug 1201535 - Test view source of srcdoc frames. r=mconley Bug 1201535 - Test view source of srcdoc frames. r=mconley

https://hg.mozilla.org/integration/fx-team/rev/82fd00bfb1dd https://hg.mozilla.org/integration/fx-team/rev/83e1422aecc8 https://hg.mozilla.org/integration/fx-team/rev/9efdda8b4a18

:bz, are the fixes here sufficient to resolve all your concerns (srcdoc and extra GET requests) in this bug? If so, I intend request uplift approval and re-enable the view source tab feature as well.

I think these are sufficient, yes.

https://hg.mozilla.org/mozilla-central/rev/82fd00bfb1dd https://hg.mozilla.org/mozilla-central/rev/83e1422aecc8 https://hg.mozilla.org/mozilla-central/rev/9efdda8b4a18

Comment on attachment 8665107 [details] MozReview Request: Bug 1201535 - Open view source tabs with about:blank. r=bz Approval Request Comment [Feature/regressing bug #]: View source in tab (bug 1067325). This feature is currently disabled in release / beta because of the regressions in this bug. If approved for uplift here, I plan to re-enable for all channels in bug 1208166. [User impact if declined]: View source in tab would continue to: * make extra hidden GET requests [Describe test coverage new/current, TreeHerder]: On m-c, new tests added. [Risks and why]: Should be low with new tests. [String/UUID change made/needed]: None

Comment on attachment 8665108 [details] MozReview Request: Bug 1201535 - Mark srcdoc as safe for loading in child. r=bz Approval Request Comment Approval Request Comment [Feature/regressing bug #]: View source in tab (bug 1067325). This feature is currently disabled in release / beta because of the regressions in this bug. If approved for uplift here, I plan to re-enable for all channels in bug 1208166. [User impact if declined]: View source in tab would continue to: * fail for srcdoc frames [Describe test coverage new/current, TreeHerder]: On m-c, new tests added. [Risks and why]: Should be low with new tests. [String/UUID change made/needed]: None

Comment on attachment 8665109 [details] MozReview Request: Bug 1201535 - Test view source of srcdoc frames. r=mconley Approval Request Comment [Feature/regressing bug #]: View source in tab (bug 1067325). This feature is currently disabled in release / beta because of the regressions in this bug. If approved for uplift here, I plan to re-enable for all channels in bug 1208166. [User impact if declined]: This patch includes tests for the srcdoc changes in this bug. [Describe test coverage new/current, TreeHerder]: On m-c, new tests added. [Risks and why]: Should be low with new tests. [String/UUID change made/needed]: None

Comment on attachment 8665107 [details] MozReview Request: Bug 1201535 - Open view source tabs with about:blank. r=bz Sure, let's try to ship 42 with this.

Hi Ryan, the 2nd patch cause problems when uplifting to beta: merging docshell/base/nsAboutRedirector.cpp warning: conflicts during merge. merging docshell/base/nsAboutRedirector.cpp incomplete! (edit conflicts, then use 'hg resolve --mark') abort: unresolved conflicts, can't continue (use hg resolve and hg graft --continue) could you take a look, thanks!

https://hg.mozilla.org/releases/mozilla-aurora/rev/3ac18b24304e https://hg.mozilla.org/releases/mozilla-aurora/rev/f79cf0398d86 https://hg.mozilla.org/releases/mozilla-aurora/rev/6458f1f2c29e

Tests were a bit different in beta, as it depending on some other not on beta. I've fixed the tests for beta, here's a try run first: https://treeherder.mozilla.org/#/jobs?repo=try&revision=92fd4d3f0e5b

http://hg.mozilla.org/releases/mozilla-beta/rev/080829e4a749 http://hg.mozilla.org/releases/mozilla-beta/rev/6c9982fcf3bc http://hg.mozilla.org/releases/mozilla-beta/rev/e46b12a6d456 Final state: * Issues fixed in 42 and later * View source tabs disabled in 41