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

RESOLVED FIXED in Firefox 41

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: bzbarsky, Assigned: jryans)

Tracking

(Blocks 1 bug)

Trunk
mozilla44
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41+ fixed, firefox42+ fixed, firefox43+ fixed, firefox44 fixed)

Details

Attachments

(5 attachments)

[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...
Flags: needinfo?(jryans)
Assignee

Comment 1

4 years ago
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
Flags: needinfo?(jryans) → needinfo?(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.
Flags: needinfo?(bzbarsky)
Assignee

Comment 3

4 years ago
(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?
Flags: needinfo?(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.
Flags: needinfo?(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.
Summary: View source in tab is totally broken for anything other than a normally-cacheable HTTP GET → View source in tab is broken for 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.
Assignee

Comment 8

4 years ago
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?
Flags: needinfo?(jgriffiths)
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.
Flags: needinfo?(jgriffiths)
Assignee

Updated

4 years ago
Keywords: leave-open
Assignee

Updated

4 years ago
Attachment #8658428 - Attachment is patch: true
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...
Attachment #8658428 - Flags: review?(bzbarsky) → review+
Assignee

Comment 14

4 years ago
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.
Attachment #8658428 - Flags: approval-mozilla-beta?
Attachment #8658428 - Flags: approval-mozilla-aurora?
Assignee

Comment 17

4 years ago
Flagging Ritu directly since there's not much time left.  Is it okay to uplift a patch to disable this feature in beta?
Flags: needinfo?(rkothari)
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)?
Flags: needinfo?(rkothari)
Flags: needinfo?(jryans)
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky)
(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.
Attachment #8658428 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
Flags: needinfo?(sledru)
Assignee

Comment 23

4 years ago
(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.
Assignee

Comment 24

4 years ago
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.
Flags: needinfo?(jryans)
(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.  :(
Assignee

Comment 28

4 years ago
Attachment #8658883 - Flags: review+
Assignee

Updated

4 years ago
Assignee

Updated

4 years ago
Attachment #8658428 - Flags: checkin+
Assignee

Updated

4 years ago
Attachment #8658883 - Flags: checkin+
Assignee

Updated

4 years ago
Attachment #8658428 - Attachment description: Disable view source on Release / Beta → Disable view source tabs on Release / Beta
Assignee: nobody → jryans
Flags: needinfo?(sledru)
Comment on attachment 8658428 [details] [diff] [review]
Disable view source tabs on Release / Beta

OK, let's do that in 42 too then.
Attachment #8658428 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee

Comment 33

4 years ago
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.
Flags: needinfo?(rkothari)
This was removed from the 41 release notes.
Flags: needinfo?(rkothari)
Assignee

Comment 36

4 years ago
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.
Attachment #8665107 - Flags: review?(bzbarsky)
Assignee

Comment 37

4 years ago
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.
Attachment #8665108 - Flags: review?(bzbarsky)
Assignee

Comment 38

4 years ago
Bug 1201535 - Test view source of srcdoc frames. r=mconley
Attachment #8665109 - Flags: review?(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.
Attachment #8665109 - Flags: review?(mconley) → review+
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
Attachment #8665107 - Flags: review?(bzbarsky) → review+
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
Attachment #8665108 - Flags: review?(bzbarsky) → review+
Assignee

Comment 43

4 years ago
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.
Assignee

Comment 44

4 years ago
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.
Assignee

Comment 45

4 years ago
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.
Assignee

Comment 46

4 years ago
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
Assignee

Comment 48

4 years ago
: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.
Flags: needinfo?(bzbarsky)
I think these are sufficient, yes.
Flags: needinfo?(bzbarsky)
Assignee

Updated

4 years ago
Keywords: leave-open
Assignee

Updated

4 years ago
Blocks: 1208166
Assignee

Comment 51

4 years ago
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
Attachment #8665107 - Flags: approval-mozilla-beta?
Attachment #8665107 - Flags: approval-mozilla-aurora?
Assignee

Comment 52

4 years ago
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
Attachment #8665108 - Flags: approval-mozilla-beta?
Attachment #8665108 - Flags: approval-mozilla-aurora?
Assignee

Comment 53

4 years ago
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
Attachment #8665109 - Flags: approval-mozilla-beta?
Attachment #8665109 - Flags: approval-mozilla-aurora?
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.
Attachment #8665107 - Flags: approval-mozilla-beta?
Attachment #8665107 - Flags: approval-mozilla-beta+
Attachment #8665107 - Flags: approval-mozilla-aurora?
Attachment #8665107 - Flags: approval-mozilla-aurora+
Attachment #8665108 - Flags: approval-mozilla-beta?
Attachment #8665108 - Flags: approval-mozilla-beta+
Attachment #8665108 - Flags: approval-mozilla-aurora?
Attachment #8665108 - Flags: approval-mozilla-aurora+
Attachment #8665109 - Flags: approval-mozilla-beta?
Attachment #8665109 - Flags: approval-mozilla-beta+
Attachment #8665109 - Flags: approval-mozilla-aurora?
Attachment #8665109 - Flags: approval-mozilla-aurora+
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!
Flags: needinfo?(jryans)
Assignee

Comment 57

4 years ago
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

Updated

4 years ago
Blocks: 1190179
Assignee

Updated

4 years ago
Summary: View source in tab is broken for srcdoc iframes → View source in tab is broken for srcdoc iframes / sends extra GET requests

Updated

4 years ago
Depends on: 1213693
You need to log in before you can comment on or make changes to this bug.