1201535 - View source in tab is broken for srcdoc iframes / sends extra GET requests

Status: RESOLVED FIXED

(Toolkit :: View Source, defect)

Description

[Boris Zbarsky [:bzbarsky]]

[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...

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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

Comment 2

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(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?

Comment 4

[Boris Zbarsky [:bzbarsky]]

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.

Comment 5

[Boris Zbarsky [:bzbarsky]]

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.

Comment 6

[Boris Zbarsky [:bzbarsky]]

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_.

Comment 7

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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

Comment 8

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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?

Comment 9

[Boris Zbarsky [:bzbarsky]]

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...

Comment 10

[Jeff Griffiths (:canuckistani) (:⚡︎)]

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 11

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Disable view source tabs on Release / Beta

Comment 12

[Boris Zbarsky [:bzbarsky]]

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 13

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/f3dc232530a5

Comment 14

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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.

Comment 15

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Try for Beta:

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

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f3dc232530a5

Comment 17

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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

Comment 18

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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)?

Comment 19

[Boris Zbarsky [:bzbarsky]]

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.

Comment 20

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(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 21

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 22

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 23

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(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.

Comment 24

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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.

Comment 25

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(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.

Comment 26

[Boris Zbarsky [:bzbarsky]]

> 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.  :(

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/997cb9b3e15d

Comment 28

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Follow up for tests

Comment 29

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/7e22b833ca50

Comment 30

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 31

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8658428 [details] [diff] [review]
Disable view source tabs on Release / Beta

OK, let's do that in 42 too then.

Comment 32

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Pushed to aurora:

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

Comment 33

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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.

Comment 34

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/7e22b833ca50

Comment 35

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

This was removed from the 41 release notes.

Comment 36

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: 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 37

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: 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 38

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: MozReview Request: Bug 1201535 - Test view source of srcdoc frames. r=mconley

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

Comment 39

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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

Comment 40

[Mike Conley (:mconley) (:⚙️)]

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 41

[Boris Zbarsky [:bzbarsky]]

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 42

[Boris Zbarsky [:bzbarsky]]

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

Comment 43

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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 44

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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 45

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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 46

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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

Comment 47

[Pulsebot]

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

Comment 48

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

: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.

Comment 49

[Boris Zbarsky [:bzbarsky]]

I think these are sufficient, yes.

Comment 50

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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 51

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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 52

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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 53

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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 54

[Sylvestre Ledru [:Sylvestre]]

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.

Comment 55

[Carsten Book [:Tomcat]]

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!

Comment 56

[Carsten Book [:Tomcat]]

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

Comment 57

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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

Comment 58

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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