1194764 - view frame source of anything interesting not working in non-e10s mode

Status: VERIFIED FIXED

(Toolkit :: View Source, defect)

Description

[Christoph Kerschbaumer [:ckerschb]]

Here is a testcase; it should be possible to view the source of all 3 iframes:
> https://bugzilla.mozilla.org/attachment.cgi?id=8365919

Regression, most likely caused by:
> Bug 1025146 seems to be the most likely candidate.

Here is the pushlog:
> https://hg.mozilla.org/integration/mozilla-inbound/
> pushloghtml?fromchange=c6a3db6682e0&tochange=060f
> 9a5bdcaf

When fixing we also wanna make sure that:
toolkit/components/viewsource/test/browser/browser_bug464222.js
is still working, not completely related, but closely related.

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Mike, any chance you could take a look at that?

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Hey Ryan, since Mike is probably not going to look into that soonish, any chance you could take a look? I would like to land Bug 1193552, but maybe I just shouldn't wait on this bug, what do you think?

Comment 3

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

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #1)
> Mike, any chance you could take a look at that?

Oh man, that was really dickish of me for clearing that needinfo without a comment. I don't know how that happened, and I'm sorry.

Yes, I'm going to be heading out for some extended PTO. jryans is a good second to look at this. Enn is another good choice.

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Mike Conley (:mconley) - (Going dark Aug 20 - Sept 11) from comment #3)
> (In reply to Christoph Kerschbaumer [:ckerschb] from comment #1)
> > Mike, any chance you could take a look at that?
> 
> Oh man, that was really dickish of me for clearing that needinfo without a
> comment. I don't know how that happened, and I'm sorry.

Don't worry about it, we got you covered :-)

Comment 5

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

Filed bug 1188752 to make it harder to clear a needinfo without a comment.

Comment 6

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

Uh, whoops. Let's try that again.

Filed bug 1195970 to make it harder to clear a needinfo without a comment.

Comment 7

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

Hmm, I am not sure the regression range is quite right...

If I set view_source.tab to false in about:config (back to window mode), then "View Frame Source" works for 3 frames. :ckerschb, is that the behavior you see too?

In view source tab mode (the default), we start a new tab based on the frame's URL and prepend "view-source:".  This is the step that seems to break down, since we lose the content of the srcdoc by only getting "about:srcdoc" as the URL.

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #7)
> Hmm, I am not sure the regression range is quite right...
> 
> If I set view_source.tab to false in about:config (back to window mode),
> then "View Frame Source" works for 3 frames. :ckerschb, is that the behavior
> you see too?
> 
> In view source tab mode (the default), we start a new tab based on the
> frame's URL and prepend "view-source:".  This is the step that seems to
> break down, since we lose the content of the srcdoc by only getting
> "about:srcdoc" as the URL.

Since James wrote the test and also queried the regression range [1], he is probably in a better position to answer that question. James?

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1193552#c11

Comment 9

[James Kitchener (:jkitch)]

I've tried the regression run again with view_source.tab set to false and I get the same result.

The transition from ownerDoc to a URL reference seems to happen in https://hg.mozilla.org/integration/mozilla-inbound/rev/28c41c53d0c2  and https://hg.mozilla.org/integration/mozilla-inbound/rev/13bf99d217bf#l5.250, which fall within the identified regression range.


That being said, I'm not an expert on the Javascript side of the codebase.

Comment 10

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

(In reply to James Kitchener (:jkitch) from comment #9)
> I've tried the regression run again with view_source.tab set to false and I
> get the same result.
> 
> The transition from ownerDoc to a URL reference seems to happen in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/28c41c53d0c2  and
> https://hg.mozilla.org/integration/mozilla-inbound/rev/13bf99d217bf#l5.250,
> which fall within the identified regression range.
> 
> 
> That being said, I'm not an expert on the Javascript side of the codebase.

What about the precise steps you are performing here?  In current Nightly 43, with view_source.tab == false, I did the following:

1. Open https://bug964239.bmoattachments.org/attachment.cgi?id=8365919
2. Right-click inside any of 3 frames
3. Choose This Frame -> View Frame Source
4. A new window appears, displaying the source of the frame as expected

James, are you using the same steps, or something else?

Comment 11

[James Kitchener (:jkitch)]

It is profile related.

My two year old test profile: Bad
One created today:  Good

I'm looking into the differences between them.

Comment 12

[James Kitchener (:jkitch)]

e10s: good
no-e10s: bad

Comment 13

[Boris Zbarsky [:bzbarsky]]

Seems like we should fix this, since no-e10s is what we're actually shipping...

Comment 14

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

(In reply to Boris Zbarsky [:bz] from comment #13)
> Seems like we should fix this, since no-e10s is what we're actually
> shipping...

I agree we should try to resolve this, especially since it's a regression.  I don't believe I personally will have time to dive into this for a few weeks at least, though.

It seems like there are several issues at play:

* e10s off causes some failure
* view source tab (instead of window) fails because it gets a bad URL for the frame content

For the tab portion, what's a good way to make srcdoc content accessible as a URL?  Should the srcdoc content be turned into a data URL?

Comment 15

[Boris Zbarsky [:bzbarsky]]

[Tracking Requested - why for this release]:

[Tracking Requested - why for this release]:

[Tracking Requested - why for this release]:

> * view source tab (instead of window) fails because it gets a bad URL for the frame
> content

The srcdoc url can't represent the actual source, obviously.  You need to pass in the source.  Of course in general the URL can't represent the actual source in all sorts of cases (think POST data, or HTTP GET that has no-cache or no-store headers).  So if the UI really is just prepending "view-source:" to the URL and not doing the things it should be doing with page descriptors, it's going to be in for a bad time in a lot of situations.

Looking at the various codepaths, here is how we end up in the docshell code in the following cases:

1)  view-source window, e10s: viewSource-content.js has a loadSource function that calls nsDocShell::LoadPage and passes in the right page descriptor.  The docshell code extracts the srcdoc data from that descriptor.  This is the right way to do view-source, and it works correctly.

2)  view-source tab, e10s: BrowserViewSourceOfDocument calls loadOneTab with the "view-source:about:srcdoc" URI, then we do a normal load.  There is no page descriptor passed in.  I get an error page in this case, as expected, since "about:srcdoc" is not a URI that's allowed to be loaded.  Looking at this codepath, it does not seem to 

3)  view-source window, non-e10s: viewSource-content.js loadSource is called again, but without a page descriptor.  That's because the "viewSource" function in that same file is called with an undefined outerWindowID, so neer finds the right page descriptor.  Whoever sent that "ViewSource:LoadSource" message didn't include an outer window id in it, apparently.

4)  view-source tab, non-e10s: Same thing as the e10s case.

So it looks to me like there are two separate bugs here:

First bug, presumably caused by bug 1025146, is that the view-source window is busted in non-e10s mode: it's not passing in the right page descriptor.  This is a problem in beta, and we need to fix this ASAP.  mconley is out, you're the reviewer on bug 1025146, so you need to do this or find someone else to do it.  I'm going to focus this bug on this problem.

Second bug: view-source in tab is just broken because it doesn't do the right page descriptor things.  This will break for all sorts of cases and needs to be fixed.  Filed bug 1201535 on this.

> For the tab portion, what's a good way to make srcdoc content accessible as a URL?

A URL can't represent view-source, period.  You need to use a page descriptor.  And if you do, docshell will do the right thing with srcdoc for you.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Ah, so this may be limited to "view frame source" only, assuming I found the right problem.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Yeah, so nsContextMenu.js is full of gContextMenuContentData.frameOuterWindowID that should be this.frameOuterWindowID.  The former is only set in e10s mode; the latter set in both modes.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Ah, and some of them were in fact landed for bug 1025146.  So now all is clear.

Comment 19

[Boris Zbarsky [:bzbarsky]]

And the rest were added in bug 1140859, breaking some of the "save frame" stuff in non-e10s mode.. :(

Comment 20

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix, without any tests or anything

I did verify that this fixes "view frame source" for me with the view-source window.

Comment 21

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

Thank you for this detailed investigation!  I would never have figured this all out so quickly.

I'll check your patch now.

Comment 22

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

Comment on attachment 8656626 [details] [diff] [review]
Fix, without any tests or anything

Review of attachment 8656626 [details] [diff] [review]:
-----------------------------------------------------------------

Great, thank you for this fix!

In my testing this fixes all frame source issues (e10s on or off) when view source *windows* are used.

As described elsewhere, we'll use bug 1201535 to resolve the view source tab specific problems.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Would you like a followup bug for adding tests for this stuff too?

Comment 24

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

(In reply to Boris Zbarsky [:bz] from comment #23)
> Would you like a followup bug for adding tests for this stuff too?

Yes, that would be great.  I can make an attempt at that once I've got more time available.

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6cb2ada75b2d

Comment 26

[Boris Zbarsky [:bzbarsky]]

Filed bug 1201773 for the tests.

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6cb2ada75b2d

Comment 28

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8656626 [details] [diff] [review]
Fix, without any tests or anything

Approval Request Comment
[Feature/regressing bug #]: Bug 1025146
[User impact if declined]: View frame source (and possibly saving of frames)
   doesn't work correctly in the (default) non-e10s mode.
[Describe test coverage new/current, TreeHerder]: Test coverage is poor.  We've
   done some manual testing, of course.
[Risks and why]: This is very low risk.
[String/UUID change made/needed]: None.

Comment 29

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

Comment on attachment 8656626 [details] [diff] [review]
Fix, without any tests or anything

Fix seems simple enough, let's uplift to Aurora42 and Beta41.

Comment 30

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

Christoph, could you please verify this fix works as expected on 9/5 Nightly or later? Thanks in advance.

Comment 31

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/75bd2fc8c22a

Comment 32

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

https://hg.mozilla.org/releases/mozilla-beta/rev/cb4ecf5ce34a

Comment 33

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Ritu Kothari (:ritu) from comment #30)
> Christoph, could you please verify this fix works as expected on 9/5 Nightly
> or later? Thanks in advance.

After chatting with JRyans on IRC I can now verify that after setting:
> "view_source.tab" = true
the testcase from Comment 0 [1] now works in both e10s and non-e10s.

It would still be great to have a fix for the tab specific case of srcdoc, but I can live with that for now. Boris filed Bug 1201535 to investigate and fix that.

[1] https://bugzilla.mozilla.org/attachment.cgi?id=8365919

Comment 34

[Petruta Horea [:phorea], Desktop Test Engineering]

Please see my results obtained under Win 7 64-bit below:

 1. Using latest Nightly 43.0a1, view_source.tab - TRUE by default, e10s & non e10s:
  a. choosing "View frame source" while the link inside iframe is not clicked opens a new tab with view-source:about:srcdoc url (The address isn't valid)
  b. opening the good/bad link and then selecting to view the source, opens the source in a new tab (eg view-source:about:mozilla)

 2. Using latest Nightly 43.0a1, view_source.tab - switched to FALSE, e10s & non e10s:
  a. choosing "View frame source" while the link inside iframe is not clicked opens a new window named Source of: about:srcdoc containing the source (eg <a href="https://bugzilla.mozilla.org">good</a>)
  b. opening the good/bad link and then selecting to view the source, opens the source in a new window

Are these the expected results? If so, I will continue the verification on aurora and beta.

Is 1.a going to be fixed in bug 1201535 where the pref will be set to false?

Thanks!

Comment 35

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

(In reply to Petruta Rasa [QA] [:petruta] from comment #34)
> Please see my results obtained under Win 7 64-bit below:
> 
>  1. Using latest Nightly 43.0a1, view_source.tab - TRUE by default, e10s &
> non e10s:
>   a. choosing "View frame source" while the link inside iframe is not
> clicked opens a new tab with view-source:about:srcdoc url (The address isn't
> valid)
>   b. opening the good/bad link and then selecting to view the source, opens
> the source in a new tab (eg view-source:about:mozilla)
> 
>  2. Using latest Nightly 43.0a1, view_source.tab - switched to FALSE, e10s &
> non e10s:
>   a. choosing "View frame source" while the link inside iframe is not
> clicked opens a new window named Source of: about:srcdoc containing the
> source (eg <a href="https://bugzilla.mozilla.org">good</a>)
>   b. opening the good/bad link and then selecting to view the source, opens
> the source in a new window
> 
> Are these the expected results? If so, I will continue the verification on
> aurora and beta.

Yes, that is the expected current state.

> Is 1.a going to be fixed in bug 1201535 where the pref will be set to false?

Yes, 1.a. should be resolved in beta soon by disabling view source tabs there.

Comment 36

[Petruta Horea [:phorea], Desktop Test Engineering]

Environments used: Win 7 64-bit and Mac OS X 10.9.5.

Verified with Firefox 41 beta 9 where the pref view_source.tab is false by default and obtained the same results as detailed in comment 34.

Based on Ryan's answer (thanks!), I'm marking 41 and 43 versions as verified and I'm removing the qe-verify+ flag.