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

VERIFIED FIXED in Firefox 41

Status

()

defect
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: ckerschb, Assigned: bzbarsky)

Tracking

(Depends on 1 bug, {regression})

unspecified
mozilla43
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s-, firefox41+ verified, firefox42+ fixed, firefox43+ verified)

Details

Attachments

(1 attachment)

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.
Blocks: 1025146, 1193552
Keywords: regression
Mike, any chance you could take a look at that?
Flags: needinfo?(mconley)
Flags: needinfo?(mconley)
Component: Document Navigation → View Source
Product: Core → Toolkit
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?
Flags: needinfo?(jryans)
(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.
(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 :-)
Uh, whoops. Let's try that again.

Filed bug 1195970 to make it harder to clear a needinfo without a comment.
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.
Flags: needinfo?(jryans) → needinfo?(mozilla)
(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
Flags: needinfo?(mozilla) → needinfo?(jkitch.bug)
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.
Flags: needinfo?(jkitch.bug)
(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?
Flags: needinfo?(jkitch.bug)
It is profile related.

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

I'm looking into the differences between them.
Flags: needinfo?(jkitch.bug)
e10s: good
no-e10s: bad
Seems like we should fix this, since no-e10s is what we're actually shipping...
Flags: needinfo?(jryans)
(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?
Flags: needinfo?(jryans)
[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.
Flags: needinfo?(jryans)
Summary: view source of srcdoc not working → view source of anything interesting not working in non-e10s mode
Ah, so this may be limited to "view frame source" only, assuming I found the right problem.
Summary: view source of anything interesting not working in non-e10s mode → view frame source of anything interesting not working in non-e10s mode
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.
Ah, and some of them were in fact landed for bug 1025146.  So now all is clear.
And the rest were added in bug 1140859, breaking some of the "save frame" stuff in non-e10s mode.. :(
I did verify that this fixes "view frame source" for me with the view-source window.
Attachment #8656626 - Flags: review?(jryans)
Thank you for this detailed investigation!  I would never have figured this all out so quickly.

I'll check your patch now.
Flags: needinfo?(jryans)
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.
Attachment #8656626 - Flags: review?(jryans) → review+
Would you like a followup bug for adding tests for this stuff too?
Flags: needinfo?(jryans)
(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.
Flags: needinfo?(jryans)
Filed bug 1201773 for the tests.
Depends on: 1201773
https://hg.mozilla.org/mozilla-central/rev/6cb2ada75b2d
Assignee: nobody → bzbarsky
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
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.
Attachment #8656626 - Flags: approval-mozilla-beta?
Attachment #8656626 - Flags: approval-mozilla-aurora?
Comment on attachment 8656626 [details] [diff] [review]
Fix, without any tests or anything

Fix seems simple enough, let's uplift to Aurora42 and Beta41.
Attachment #8656626 - Flags: approval-mozilla-beta?
Attachment #8656626 - Flags: approval-mozilla-beta+
Attachment #8656626 - Flags: approval-mozilla-aurora?
Attachment #8656626 - Flags: approval-mozilla-aurora+
Christoph, could you please verify this fix works as expected on 9/5 Nightly or later? Thanks in advance.
Flags: needinfo?(mozilla)
Flags: qe-verify+
(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
Flags: needinfo?(mozilla)
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!
(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.
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
See Also: → 1213693
You need to log in before you can comment on or make changes to this bug.