1608224 - Source for Twitter page not shown as expected

Status: VERIFIED FIXED

(Core :: DOM: Networking, defect, P2)

Description

[Yaron Tausky [:ytausky]]

When I go to someone's Twitter page (example) and choose "View Page Source" from the context menu, I get a non-styled rendering of the page itself instead of the normal source viewer.

Comment 1

[Michael Kohler [:mkohler]]

I'm on Nightly Build ID 20200107095722 and it works for me. So you can treat that as lower range regression window point.

Comment 2

[:Gijs (he/him)]

Can you reproduce on an empty profile? I can't, and I wonder if this is a dupe of bug 1605657.

Comment 3

[:Gijs (he/him)]

(In reply to :Gijs (he/him) from comment #2)

Can you reproduce on an empty profile? I can't, and I wonder if this is a dupe of bug 1605657.

Oh, sorry, I can reproduce, but reloading the view source page (which I did after uninstalling the add-on that trips bug 1605657) fixes it.

Comment 4

[Cristian Fogel [:cfogel]]

Mozregression points out to this range.

• last good build_date: 2020-01-07;
• first bad build_date: 2020-01-08.
• 73.0b3 not affected by this.

Comment 5

[:Gijs (he/him)]

Probably bug 1598523? Matt, could you take a look?

Comment 6

[Matt Woodrow (:mattwoodrow)]

Hmm, looks like view-source is broken for anything that redirects, due to a serviceworker intercept or a real HTTP redirect (we need more tests for this!).

The issue is that AsyncOnChannelRedirect gets called on the DocumentLoadListener for the old and new inner channels (the real http channels, not the nsViewSourceChannel). We then grab the OriginalURI from the new channel, and store it in mChannelCreationURI, and that doesn't include 'view-source'.

When we try to create the real channel in the content process, the URI we have no longer has 'view-source', so we don't get an nsViewSourceChannel wrapper in the content process.

We take a copy of the OriginalURI during AsyncOnChannelRedirect, since it gets overwritten to be the OriginalURI of the initial (pre-redirect) load after this point. We need the value we're grabbing in order to create the real channel in the content process. We really need a variant of this that includes the 'view-source' prefix, which doesn't appear to exist anywhere right now.

nsViewSourceChannel appears to largely ignore redirects, except that it updates it's URI/mChannel when it gets OnStartRequest. This includes manually adding a 'view-source' prefix, but this is on the final URI, not the original of the current channel.

It also appears that functions like Suspend/Resume on nsViewSourceChannel are broken if we have a redirect and try to call them before OnStartRequest, since they'd try to Suspend the original channel, not the current redirect.

DocumentLoadListener caches mChannel on redirects, as well as the original URI, both of which seem like they'll be wrong when we get a redirect for the inner channel.

Honza, do you have any ideas on the best way to solve this?

I'm beginning to think we should just hide 'view-source' from the parent entirely, and just append it when we get back to the child process. I think we wanted to make process switching decision based on view-source though, which we'd no longer know about.

Comment 7

[Honza Bambas (:mayhemer)]

Thanks for the description, Matt. Something tells me this is a fallout from bug 1598523 (please adjust if it it really something else). To be honest, I really lack a good tech documentation for the docchannel code right now to look into it and recall the code flow. The code became rather very complicated and I don't have cycles to go through it and study it again to understand what to do with this bug now. I'll schedule a f2f meeting to talk about this, it will be more effective.

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

:ytausky, since this bug is a regression, could you fill (if possible) the regressed_by field?
For more information, please visit auto_nag documentation.

Comment 9

[Honza Bambas (:mayhemer)]

The idea to fix this is via another "view-source" specific hack:

• let nsViewSourceChannel implement nsIInterfaceRequestor and nsIChannelEventSink
• let it inject itself as notification callbacks of the inner channel in Init or AsyncOpen
• from GetInterface return:
  • nsIViewSourceChannel: itself
  • nsIChannelEventSink: itself
  • anything else forward to the outer (original) notification callbacks
• in AsyncOnChannelRedirect inject like this to the new channel as well and then just forward call to AsyncOnChannelRedirect to the outer notification callbacks with the same arguments
• DocumentLoadListener then can query the new redirect-to channel's notification callbacks for nsIViewSourceChannel to and update the creation-URI to view-source:, probably here
  • possibly make nsViewSourceChannel::BuildViewSourceURI a public static method to do this more easily

I decided to use a hack rather than updating new channel's original URI (or LI's rpURI) to be prefixed with "view-source:" in AsyncOnChannelRedirect to not effect security checks we are performing during redirection. It may be worth filing a followup bug, but I think it's a too sensitive change to do to just fix this particular bug. I didn't write down why I have chosen the URI update as late as in OnStartRequest in bug 1403998.

Comment 10

[Boris Zbarsky [:bzbarsky]]

in AsyncOnChannelRedirect inject like this to the new channel as well

Or create a new view-source channel wrapping the new post-redirect channel? If it did that, it could pass itself and the new thing to the outer notification callbacks, right? And then DocumentLoadListener might not need any special magic?

Comment 11

[Honza Bambas (:mayhemer)]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #10)

in AsyncOnChannelRedirect inject like this to the new channel as well

Or create a new view-source channel wrapping the new post-redirect channel? If it did that, it could pass itself and the new thing to the outer notification callbacks, right? And then DocumentLoadListener might not need any special magic?

Thanks, yes. That is another option that also crossed my mind, but that may effect security checks as well and is also a lot more work. We then may need an IPC protocol for the view-source channel (have parent and child parts), but it's more a guess at the moment - maybe we don't.

I'll try that first, but something tells me it will be more complicated and we have just few days to get this done for this, affected release... I'm not sure I will have time to work on this in Berlin.

Comment 12

[Honza Bambas (:mayhemer)]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #10)

in AsyncOnChannelRedirect inject like this to the new channel as well

Or create a new view-source channel wrapping the new post-redirect channel? If it did that, it could pass itself and the new thing to the outer notification callbacks, right? And then DocumentLoadListener might not need any special magic?

Reading this again... I'm not sure how this would help or at least I can see a simpler solution after (re-)studding our redirect notify ordering.

According the redirect notify helper logic we first notify the IO service which calls, in this order: CONTENTSECURITYMANAGER, all registered global category entries. Then it pops back to the helper to notify the callbacks of the old channel, if implementing nsIEventSink.

So, the actual update of the URL happens as the last and thus will not effect security check (unless I'm missing something). We are then probably fine to simply update the OriginalURI of the new channel in nsViewSourceChannel implementing (and forwarding) nsIEventSink. That is the proper and sufficient fix. Hopefully nothing else will break.

Comment 13

[Honza Bambas (:mayhemer)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2bc37da2fac4cff653a320f611bd5451c31beb1e

Comment 14

[Honza Bambas (:mayhemer)]

Attachment: Bug 1608224 - Update originalURI of the "new" redirect channel used by a view-source channel to let consumers see it has been actually used for view-source, r=bzbarsky

Comment 15

[Matt Woodrow (:mattwoodrow)]

I think we still might have bugs, because we're going to call DocumentLoadListener::AsyncOnChannelRedirect, which will change mChannel from the old nsViewSourceChannel to the new nsHttpChannel.

Looks like we'd set mChannel back to the nsViewSourceChannel during OnStartRequest, and most places we use it are after that.

I think only Cancel() could use the wrong mChannel (but it's confusing to verify), and that should be fine to Cancel the new inner http channel directly, so maybe everything will work.

That said, it's a bit confusing. I like the idea of creating a new nsViewSourcewChannel wrapper (after security checks are done), just so that DocumentLoadListener doesn't get confused.

Comment 16

[Honza Bambas (:mayhemer)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #15)

I think we still might have bugs, because we're going to call DocumentLoadListener::AsyncOnChannelRedirect, which will change mChannel from the old nsViewSourceChannel to the new nsHttpChannel.

Looks like we'd set mChannel back to the nsViewSourceChannel during OnStartRequest, and most places we use it are after that.

I think only Cancel() could use the wrong mChannel (but it's confusing to verify), and that should be fine to Cancel the new inner http channel directly, so maybe everything will work.

That said, it's a bit confusing. I like the idea of creating a new nsViewSourcewChannel wrapper (after security checks are done), just so that DocumentLoadListener doesn't get confused.

Can you please be more specific what exactly this would help with? Or what confusion you exactly see?

Comment 17

[Honza Bambas (:mayhemer)]

Ah... I see now. Hmm... that was there before as well and you are working more with the inner channel anyway, so I thought this would be fine to do. I can have a different patch that will create a new nsViewSourceChannel. Note that the "old" channel will then also be (for consistency) the old (current mChannel) nsV.S.C, what actually might be a better as well.

Tomorrow.

Comment 18

[Honza Bambas (:mayhemer)]

I started to write a patch according the bz's and matt's proposal and it's getting overly complicated. I will open a new bug for that work. Because I won't have time to work on it for this release, let's consider the patch here as an intermediate fix.

bz, matt, what do you think?

Comment 19

[Boris Zbarsky [:bzbarsky]]

That sounds reasonable to me.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bdad1c3a97a0
Update originalURI of the "new" redirect channel used by a view-source channel to let consumers see it has been actually used for view-source, r=bzbarsky

Comment 21

[Noemi Erli[:noemi_erli]]

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

Comment 22

[Alexandru Trif, Desktop QA [:atrif]]

Reproduced the issue using Firefox 74.0a1 (20200109213942) on Windows 10x64.
The issue is verified fixed with Firefox 74.0b5 (20200218224219) on Windows 10x64, macOS 10.15 and Ubuntu 16.04.