1025146 - [e10s] Never load the source off of the network when viewing source

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect, P3)

Description

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

Bug 988133 worked around a main-process crash that we hit if attempting to view source while using a remote tab.

The work around involves us detecting that the document we're attempting to load the source from is in a remote tab, and instead of loading the source from the cache, we re-load it off the network. This allows us to sidestep the cache, but is probably not ideal.

We should try to make the source viewer not rely on the document so much, or make it so that the CPOW'd nsIWebPageDescriptor of a CPOW'd document doesn't explode when attempting to load a page from the cache.

We should also take this opportunity to see what other browsers are doing when viewing a page source. Are they also loading off the cache, or are they reloading off the network?

Comment 1

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

Ok, I've got a plan here that I think might work.

First, we add a framescript to the view source browser. Next, we detect if the browser that we're requesting document source from is remote, and if so, make the view source browser remote as well. We'll probably want to do some work down the line to make sure that the view source browser has the same content process as the original document browser, but since there's no API (that I know of) that allows this, and since we're only supporting 1 content process at this time, a binary choice of either remote / non-remote is probably fine for a start.

We have the parent process pass the document that we're loading the source from to the framescript of the viewsource browser. In the e10s case, it's passing in a CPOW - but since the framescript runs in the same process as the original document's browser, the non-CPOW'd document shows up on the other side.

The framescript then QI's / manipulates the document to get at the nsIWebPageDescriptor descriptor for the document's docshell. It then loads that in to the view source browser's docshell, which the framescript has access to. I have a PoC that demonstrates that what I've just described is possible.

This will require some re-jigging and re-organization of the old toolkit view source code, but it shouldn't be too, too bad.

Comment 2

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

billm: you're the one who gave me the idea for this plan back in https://bugzilla.mozilla.org/show_bug.cgi?id=988133#c1 - does my plan sound like a decent way forward?

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

This sounds reasonable. Do we need a framescript though? I wonder what would happen if we just make the viewsource browser be remote and let CPOWs take care of the rest.

Comment 5

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

(In reply to Bill McCloskey (:billm) from comment #4)
> This sounds reasonable. Do we need a framescript though? I wonder what would
> happen if we just make the viewsource browser be remote and let CPOWs take
> care of the rest.

The nsIWebPageDescriptor interface which is what allows us to load things out of the bfcache is only available on the DocShell of the browser, so if we're using a remote tab, we have to use a frame script in order to get access to the DocShell / nsIWebPageDescriptor interface to call loadPage on it.

At least, I think we have to - unless there's an alternative I'm not considering.

Comment 6

[Chris Peterson [:cpeterson]]

Move old M2 P3 bugs to M4 (because tracking-e10s=m5+ flag doesn't exist yet).

Comment 7

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

Attachment: [e10s] WIP - Make it so that we can view document source without hitting the network. r=?

Comment 8

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

Some local testing shows that even before this patch, we seldom if ever hit the network when viewing source.

I wonder how that works - with e10s, we _should_ be skipping the nsIWebPageDescriptor loadPage thing, so I'm not sure how we're getting the document source without network activity.

I've confirmed this with a local server with traffic logging. Really quite strange.

Regardless, we might want to (again) downgrade the severity of this one.

Comment 9

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

I'd still like to understand this to make sure there aren't some edge-cases where we end up loading the page off the network. Still though, it seems like in the general case we don't load off the network. Downgrading priority.

Comment 10

[:Felipe Gomes (needinfo for replies!)]

My guess is that what we're bypassing is the bfcache, but we still end up loading the page from the network cache. Did you test setting the http Cache-Control heads on the page? I think you mentioned you did, but there are so many values for it that mean different things, so maybe the combination wasn't enough.

one way to test this would be to change the LOAD_FLAGS_NONE to LOAD_FLAGS_BYPASS_CACHE here: mxr.mozilla.org/mozilla-central/source/toolkit/components/viewsource/content/viewSource.js#177

Comment 11

[nemo]

So, maybe this is obvious, but, yeah, to answer the question at the end of the bug, AFAIK no other browser refetches off the network, not even
non-e10s Firefox.
People kinda expect the source one sees in ctrl-u to be the source one has in front of one.

Things that screw up the current approach...
POST
Page using random()

Here's a website that uses both that is messed up with e10s.
http://www.pangloss.com/seidel/Poem/

Comment 12

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

Ok, yes, I can reproduce this using that page that nemo referred to in comment 11. Thanks nemo!

Comment 13

[[e-mail]]

Another approach might be to serialise the session history entry for which you want to view the source (SessionHistory.jsm already does this obviously) and then deserialise it in the context of the view source window so that when you view the source it loads the same cache entry (because you serialised the cache key).

Comment 14

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

(In reply to [e-mail] from comment #13)
> Another approach might be to serialise the session history entry for which
> you want to view the source (SessionHistory.jsm already does this obviously)
> and then deserialise it in the context of the view source window so that
> when you view the source it loads the same cache entry (because you
> serialised the cache key).

I've looked into this, and the problem appears to be that the nsISHEntry I'm attempting to serialize has a null cache key... this appears to be the case because nsDocShell::AddToSessionHistory attempts to QI the passed in nsIChannel to an nsICachingChannel - but the nsIChannel in the multi-process case is an HttpChannelChild.cpp, which does not implement that interface. So no cache key is retrieved from it, and no cache key is set on the nsISHEntry.

I asked around in #necko, and apparently, I want to talk to mayhemer about this.

Hey mayhemer - suppose I have an nsISHEntry in the content process that I got a handle on via nsIWebPageDescriptor.currentDescriptor of the outer window. Should that nsISHEntry have a cacheKey? Should HttpChannelChild.cpp be implementing the nsICachingChannel interface?

Comment 15

[[e-mail]]

(In reply to Mike Conley from comment #14)
> (In reply to comment #13)
> > Another approach might be to serialise the session history entry for which
> > you want to view the source (SessionHistory.jsm already does this obviously)
> > and then deserialise it in the context of the view source window so that
> > when you view the source it loads the same cache entry (because you
> > serialised the cache key).
> 
> I've looked into this, and the problem appears to be that the nsISHEntry I'm
> attempting to serialize has a null cache key... this appears to be the case
> because nsDocShell::AddToSessionHistory attempts to QI the passed in
> nsIChannel to an nsICachingChannel - but the nsIChannel in the multi-process
> case is an HttpChannelChild.cpp, which does not implement that interface. So
> no cache key is retrieved from it, and no cache key is set on the nsISHEntry.

That sounds bad... does that affect other uses of the cache key, such the Back button or Sesssion Restore?

Comment 16

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I don't think the back button uses cacheKey. Session store saves and restores the cache key, but doesn't otherwise touch it. I guess we should make sure that restoring a session will load pages from the cache, rather than the network, when possible. I think I've already seen it doing that, though, so I'm not too worried.

Comment 17

[Honza Bambas (:mayhemer)]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #14)
> Hey mayhemer - suppose I have an nsISHEntry in the content process that I
> got a handle on via nsIWebPageDescriptor.currentDescriptor of the outer
> window. Should that nsISHEntry have a cacheKey? 

The cache is inaccessible on content process.

> Should HttpChannelChild.cpp
> be implementing the nsICachingChannel interface?

Probably too hard.  There is no remoting in the http cache at all.  All cache acces/write/whatever happens solely on the parent process.

Probably write IPC for the view-source channel?  That will read from cache on the parent and IPC back to child?  Does it need to support redirect (I should know this - just would need to take a look into the code)?

Comment 18

[Jason Duell]

Honza:  is there any reason we can't move just the .cacheKey field from nsICachingChannel to nsICacheInfo channel (and send it across in SendOnStartRequest, and if set in child, during AsyncOpen)?  From a glance that doesn't look impossible, and then all we'd need to do is change the Docshell to QI the channel to cacheInfo:

   https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp?from=nsDocShell.cpp#10726

Comment 19

[Jason Duell]

(We would also need to change whatever sets the mCacheKey field in the mLSHE/mOSHE structs.  But that "just" a matter of poking through the docshell to see where it gets it from the original channel)

Comment 20

[Jason Duell]

I filed bug 1156493 for the cacheKey work.  Feel free to close it if I'm wrong about the solution here Honza.

Comment 21

[Honza Bambas (:mayhemer)]

(In reply to Jason Duell [:jduell] (needinfo? me) from comment #18)
> Honza:  is there any reason we can't move just the .cacheKey field from
> nsICachingChannel to nsICacheInfo channel (and send it across in
> SendOnStartRequest, and if set in child, during AsyncOpen)?  From a glance
> that doesn't look impossible, and then all we'd need to do is change the
> Docshell to QI the channel to cacheInfo:
> 
>   
> https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.
> cpp?from=nsDocShell.cpp#10726

This, what you refer to, just sets the post id [1].  I'm not sure this is what is needed for this bug.  I'm afraid comment 17 still applies.  If you feel not, I would have to dive deeper in this (no time now).

[1] http://hg.mozilla.org/mozilla-central/annotate/50b95032152c/netwerk/protocol/http/nsHttpChannel.cpp#l6105

Comment 22

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

(In reply to Honza Bambas (:mayhemer) from comment #17)
> (In reply to Mike Conley (:mconley) - Needinfo me! from comment #14)
> > Hey mayhemer - suppose I have an nsISHEntry in the content process that I
> > got a handle on via nsIWebPageDescriptor.currentDescriptor of the outer
> > window. Should that nsISHEntry have a cacheKey? 
> 
> The cache is inaccessible on content process.

Ok, that's fine. But the content process needs to access the network to get the document source, and presumably it is able to hit the network cache (by way of the HttpChannel IPC proxy). Can we not somehow load the source in the content process in the same way? Through the proxy? Or am I misunderstanding how this all works?

> 
> > Should HttpChannelChild.cpp
> > be implementing the nsICachingChannel interface?
> 
> Probably too hard.  There is no remoting in the http cache at all.  All
> cache acces/write/whatever happens solely on the parent process.
> 
> Probably write IPC for the view-source channel?  That will read from cache
> on the parent and IPC back to child?  Does it need to support redirect (I
> should know this - just would need to take a look into the code)?

This is where my understanding of Necko kinda breaks down. There's a separate channel type for view-source? Note that the view-source "protocol" already works for the content process - example:

view-source:https://www.mozilla.org/en-US/

^-- this will load in the content process.

It's just that for these pages that are the result of POST requests...we send another GET to the URL instead of retrieving the source after the original POST.

Is any of this useful for determining what the correct course of action is?

Comment 23

[Michal Novotny [:michal]]

Attachment: backtrace

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #22)
> This is where my understanding of Necko kinda breaks down. There's a
> separate channel type for view-source? Note that the view-source "protocol"
> already works for the content process - example:
> 
> view-source:https://www.mozilla.org/en-US/
> 
> ^-- this will load in the content process.

nsViewSourceChannel is instantiated in the content process, but it initializes HttpChannelChild for the actual work (see attached backtrace), so IMO bug 1156493 should probably fix this.

Comment 24

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

(In reply to Michal Novotny (:michal) from comment #23)
> Created attachment 8595253 [details]
> backtrace
> 
> (In reply to Mike Conley (:mconley) - Needinfo me! from comment #22)
> > This is where my understanding of Necko kinda breaks down. There's a
> > separate channel type for view-source? Note that the view-source "protocol"
> > already works for the content process - example:
> > 
> > view-source:https://www.mozilla.org/en-US/
> > 
> > ^-- this will load in the content process.
> 
> nsViewSourceChannel is instantiated in the content process, but it
> initializes HttpChannelChild for the actual work (see attached backtrace),
> so IMO bug 1156493 should probably fix this.

Is there a minimal PoC patch we could apply to prove that it will fix it?

Comment 25

[Michal Novotny [:michal]]

I'm going to work on it now.

Comment 26

[Michal Novotny [:michal]]

(In reply to Michal Novotny (:michal) from comment #23)
> nsViewSourceChannel is instantiated in the content process, but it
> initializes HttpChannelChild for the actual work (see attached backtrace),
> so IMO bug 1156493 should probably fix this.

This my comment is completely irrelevant. This is true only in case you type the view-source URL into the address bar. When displaying source via ctrl-U, nsDocShell::DoURILoad() is called in the parent process instead of the child process. I have no idea how this is supposed to work.

If we fix bug 1156493, we will have a cacheKey in the SHEntry in the child process. Who is going to set the correct SHEntry in the parent process?

Comment 27

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

(In reply to Michal Novotny (:michal) from comment #26)
> (In reply to Michal Novotny (:michal) from comment #23)
> > nsViewSourceChannel is instantiated in the content process, but it
> > initializes HttpChannelChild for the actual work (see attached backtrace),
> > so IMO bug 1156493 should probably fix this.
> 
> This my comment is completely irrelevant. This is true only in case you type
> the view-source URL into the address bar. When displaying source via ctrl-U,
> nsDocShell::DoURILoad() is called in the parent process instead of the child
> process. I have no idea how this is supposed to work.

I have a work-in-progress patch to load the source in a remote browser in the View Source window, so the nsIWebDescriptor.loadPage (which eventually calls the DoURILoad in the DocShell) will be operating in the content process.

> 
> If we fix bug 1156493, we will have a cacheKey in the SHEntry in the child
> process. Who is going to set the correct SHEntry in the parent process?

I'm hoping that we can just load the source in the content process.

Comment 28

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

Let me post an updated WIP that we can test a cache key patch against. One moment.

Comment 29

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

Attachment: MozReview Request: bz://1025146/mconley

/r/7403 - [WIP] Bug 1025146 - Modernize ViewSourceUtils to be able to use outerWindowIDs to load document sources. r=?
/r/7405 - [WIP] Bug 1025146 - Modernize viewSource.js to use a frame script. r=?

Pull down these commits:

hg pull -r 65811b5bf9d81368a34f82e0c7f37a81b9a56ce0 https://reviewboard-hg.mozilla.org/gecko/

Comment 30

[Barret Rennie [:barret] (they/them)]

https://reviewboard.mozilla.org/r/7405/#review6173

::: browser/base/content/browser.js:2305
(Diff revision 1)
> -      // If nsIWebNavigation cannot be found, just get the one for the whole
> +  if (aArgsOrDocument) {

Won't this be true in the case where `aArgsOrDocument` is an object?

According to my console:

```
< Boolean({'URL': 'http://foo'})
> True

< Boolean({})
> True
```

Comment 31

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

Adding devedition-40 whiteboard so we can track this issue as it relates to view-source in tabs, see bug 1067325

Comment 32

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

https://reviewboard.mozilla.org/r/7405/#review6203

> Won't this be true in the case where `aArgsOrDocument` is an object?
> 
> According to my console:
> 
> ```
> < Boolean({'URL': 'http://foo'})
> > True
> 
> < Boolean({})
> > True
> ```

That's a good call - thanks brennie. This stuff is in dire need of more testing and refinement.

Comment 33

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

Comment on attachment 8595559 [details]
MozReview Request: bz://1025146/mconley

/r/7403 - [WIP] Bug 1025146 - Modernize ViewSourceUtils to be able to use outerWindowIDs to load document sources. r=?
/r/7405 - [WIP] Bug 1025146 - Modernize viewSource.js to use a frame script. r=?

Pull down these commits:

hg pull -r 455acd515f8e67023b65c959e397c87ca5de9eb5 https://reviewboard-hg.mozilla.org/gecko/

Comment 34

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

https://reviewboard.mozilla.org/r/7403/#review6209

::: toolkit/components/viewsource/content/viewSourceUtils.js:46
(Diff revision 2)
> +   *        The content document we would like to view the source of.

I should mention that this will throw if the document is a CPOW.

Comment 35

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

https://reviewboard.mozilla.org/r/7405/#review6211

::: browser/base/content/browser.js:2336
(Diff revision 2)
> +  top.gViewSourceUtils.viewSource({
> +    browser: browser,
> +    outerWindowID: browser.outerWindowID,
> +    URL: browser.currentURI.spec,
> +  });

Maybe this should just call BrowserViewSourceOfDocument so as to not duplicate calling viewSource on gViewSourceUtils.

::: toolkit/components/viewsource/content/viewSource.xul:228
(Diff revision 2)
> -    <browser id="content" type="content-primary" name="content" src="about:blank" flex="1"
> +    <browser id="content" type="content-primary" name="content" src="about:blank" flex="1" message="true"

message="true" is not needed.

::: toolkit/components/viewsource/content/viewSource.js:60
(Diff revision 2)
> +  let mm = gBrowser.messageManager;
> +  mm.loadFrameScript("chrome://global/content/viewSource-content.js", true);

Let's use a message manager group so that we don't have to keep reloading this framescript.

::: toolkit/components/viewsource/content/viewSource.js:74
(Diff revision 2)
> +  if (args.browser && args.browser.isRemoteBrowser) {
> +    let parentNode = gBrowser.parentNode;
> +    gBrowser.remove();
> +    gBrowser.setAttribute("remote", "true");
> +    parentNode.appendChild(gBrowser);
> +    mm = gBrowser.messageManager;
> +    mm.loadFrameScript("chrome://global/content/viewSource-content.js", true);
> +  }

We need to avoid flicker, perhaps. Maybe set a background colour on the view source window?

::: toolkit/components/viewsource/content/viewSource.js:83
(Diff revision 2)
> +  dump("\nSending the message\n");

Get rid of the dump statements!

::: toolkit/components/viewsource/content/viewSource.js:91
(Diff revision 2)
> +  dump("\nMessage sent\n");

Get rid of the dump statements!

::: toolkit/components/viewsource/content/viewSource.js:135
(Diff revision 2)
> +  };

This needs to be completed.

::: toolkit/components/viewsource/content/viewSource-content.js:14
(Diff revision 2)
> +  dump("\nGot outer window ID: " + outerWindowID);

Get rid of the dump statements in the rest of this file.

::: toolkit/components/viewsource/content/viewSourceUtils.js:38
(Diff revision 2)
> -   *        url:
> +   *        URL:

These changes should maybe be moved to the previous patch.

::: toolkit/content/browser-child.js:9
(Diff revision 2)
> +dump("\nbrowser-child.js is being loaded! This must be happening in the content process.\n");

Get rid of the dump statements!

::: toolkit/content/browser-child.js:85
(Diff revision 2)
> +

Get rid of the newline.

::: toolkit/content/widgets/remote-browser.xml:239
(Diff revision 2)
> +          dump("\nConstructing a remote browser.\n");

Get rid of the dump statements!

::: toolkit/content/widgets/remote-browser.xml:269
(Diff revision 2)
> +          dump("\nRemote browser successfully constructed\n");

Get rid of the dump statements!

Comment 36

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

Comment on attachment 8595559 [details]
MozReview Request: bz://1025146/mconley

/r/7403 - Bug 1025146 - Modernize ViewSourceUtils to be able to use outerWindowIDs to load document sources. r=?
/r/7405 - [WIP] Bug 1025146 - Modernize viewSource.js to use a frame script. r=?

Pull down these commits:

hg pull -r 0c2344712633c26d0b9475a268790ed248ce8091 https://reviewboard-hg.mozilla.org/gecko/

Comment 37

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

Comment on attachment 8595559 [details]
MozReview Request: bz://1025146/mconley

/r/7403 - Bug 1025146 - Modernize ViewSourceUtils to be able to use outerWindowIDs to load document sources. r=?
/r/7405 - [WIP] Bug 1025146 - Modernize viewSource.js to use a frame script. r=?

Pull down these commits:

hg pull -r 3edfc0e736a0865ce64f7108f32460a854eb2ad0 https://reviewboard-hg.mozilla.org/gecko/

Comment 38

[Honza Bambas (:mayhemer)]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #22)
> (In reply to Honza Bambas (:mayhemer) from comment #17)
> > (In reply to Mike Conley (:mconley) - Needinfo me! from comment #14)
> > > Hey mayhemer - suppose I have an nsISHEntry in the content process that I
> > > got a handle on via nsIWebPageDescriptor.currentDescriptor of the outer
> > > window. Should that nsISHEntry have a cacheKey? 
> > 
> > The cache is inaccessible on content process.
> 
> Ok, that's fine. But the content process needs to access the network to get
> the document source, and presumably it is able to hit the network cache (by
> way of the HttpChannel IPC proxy). Can we not somehow load the source in the
> content process in the same way? Through the proxy? Or am I misunderstanding
> how this all works?

You misunderstand.

> 
> > 
> > > Should HttpChannelChild.cpp
> > > be implementing the nsICachingChannel interface?
> > 
> > Probably too hard.  There is no remoting in the http cache at all.  All
> > cache acces/write/whatever happens solely on the parent process.
> > 
> > Probably write IPC for the view-source channel?  That will read from cache
> > on the parent and IPC back to child?  Does it need to support redirect (I
> > should know this - just would need to take a look into the code)?
> 
> This is where my understanding of Necko kinda breaks down. There's a
> separate channel type for view-source? Note that the view-source "protocol"
> already works for the content process - example:
> 
> view-source:https://www.mozilla.org/en-US/
> 
> ^-- this will load in the content process.
> 
> It's just that for these pages that are the result of POST requests...we
> send another GET to the URL instead of retrieving the source after the
> original POST.
> 
> Is any of this useful for determining what the correct course of action is?

POSTs!  OK, then the Jason's bug will fix this.  I wasn't aware from your questions this was only about POSTs.

Comment 39

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

Comment on attachment 8595559 [details]
MozReview Request: bz://1025146/mconley

/r/7403 - Bug 1025146 - Modernize ViewSourceUtils to be able to use outerWindowIDs to load document sources. r=?
/r/7405 - [WIP] Bug 1025146 - Modernize viewSource.js to use a frame script. r=?

Pull down these commits:

hg pull -r 019c47f47d9bc01b6587385bd0370a34226358d7 https://reviewboard-hg.mozilla.org/gecko/

Comment 40

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

Comment on attachment 8595559 [details]
MozReview Request: bz://1025146/mconley

/r/7403 - Bug 1025146 - Modernize ViewSourceUtils to be able to use outerWindowIDs to load document sources. r=?
/r/7405 - Bug 1025146 - Modernize viewSource.js to use a frame script. r=?
/r/7647 - Bug 1025146 - Update ViewSource tests to account for refactor. r=?

Pull down these commits:

hg pull -r 5bfe0f160faa73a67f80ac1623ca5ce8f8fb1d9c https://reviewboard-hg.mozilla.org/gecko/

Comment 41

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

Sorry jryans - I know that second commit is kinda epic. Let me know if you have any questions or problems with it.

Comment 42

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

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

Comment 43

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

https://reviewboard.mozilla.org/r/7405/#review6475

It seems good overall!  I had a few outstanding questions, and some commands aren't working yet, so I'll hold off r+ until after that.

::: browser/base/content/browser.js:2286
(Diff revision 6)
> -  // Get the document charset
> + *        Passing a document is a deprecated API that is not supported with e10s.

Again, I am curious who the potential users of the deprecated API are.

::: browser/base/content/browser.js:2309
(Diff revision 6)
> +  if (aArgsOrDocument.ownerDocument) {

Are you checking `ownerDocument` because that value is actually needed, or just as a check of "is a Document"?  It's possible for `document.ownerDocument` to be null, so it seems that would break this check, right?

Maybe instead check for the properties you need, such as `defaultView`?

::: toolkit/components/viewsource/content/viewPartialSource.xul:21
(Diff revision 6)
>          onload="onLoadViewPartialSource();"

It looks like `viewPartialSource.xul` will now run this `onload`, as well as the one bound to `load` in `viewSource.js`.  Is that correct?

::: toolkit/components/viewsource/content/viewPartialSource.xul:55
(Diff revision 6)
>    <command id="cmd_wrapLongLines" oncommand="wrapLongLines()"/>

I think this needs to be updated?  The long line menu item currently has no effect in partial mode.

::: toolkit/components/viewsource/content/viewPartialSource.xul:54
(Diff revision 6)
>    <command id="cmd_highlightSyntax" oncommand="highlightSyntax();"/>

I think this needs to be updated?  The highlighter menu item currently has no effect in partial mode.

::: toolkit/components/viewsource/content/viewSource.js:234
(Diff revision 6)
> +   *     The line number to focus on once the source is loaded.

Since you mention the deprecated API here, it might be good to list what those arguments look like for easier reading, even though it is mentioned further down.

::: toolkit/components/viewsource/content/viewSource.js:768
(Diff revision 6)
> -        if (curLine == line && !("range" in result)) {
> +// compatibility reasons. These will be removed soon.

Probably good to file a bug to do the removal, and reference it here.

::: toolkit/components/viewsource/content/viewSource-content.js:497
(Diff revision 6)
> +   *

Nit: Remove this extra blank line.

Comment 44

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

https://reviewboard.mozilla.org/r/7403/#review6469

::: toolkit/components/viewsource/content/viewSourceUtils.js:54
(Diff revision 5)
> +    // We should guarantee that aDocument is not a CPOW. Anybody attempting to

It seems like you forgot to finish this comment.

::: toolkit/components/viewsource/content/viewSourceUtils.js:28
(Diff revision 5)
> +   *        we will be supporting the old API for this method.

Who are the users of the old API we are concerned with here?  Add-ons?

Comment 45

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

https://reviewboard.mozilla.org/r/7647/#review6497

Ship It!

Comment 46

[[e-mail]]

Comment on attachment 8595559 [details]
MozReview Request: bz://1025146/mconley

> +  // TODO: Is there a better way of determining whether or not aArgsOrDocument is
> +  // in fact a document?
instanceof Document (that might not work for CPOWs, but maybe you could just check for them first).

> +    // We use the group message manager so that if we switch remoteness of the
> +    // browser (which we might do if we're attempting to load the document
> +    // source out of the network cache), we automatically re-inject the frame
> +    // frame script.
Out of interest, what was wrong with using the window message manager?

(In reply to Bill McCloskey from comment #16)
> I guess we should make sure that restoring a session will load pages from
> the cache, rather than the network, when possible. I think I've already seen
> it doing that, though, so I'm not too worried.
Per comment #22, before bug 1156493 it would have failed for POSTed pages.

(In reply to Michal Novotny from comment #26)
> If we fix bug 1156493, we will have a cacheKey in the SHEntry in the child
> process. Who is going to set the correct SHEntry in the parent process?
My comment #13 still applies, and would still work in the case of multiprocesss e10s, which I believe some developers use.

Comment 47

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

https://reviewboard.mozilla.org/r/7405/#review6551

> Again, I am curious who the potential users of the deprecated API are.

There are a few add-ons that use this, but I'm thinking primarily of SeaMonkey, Thunderbird, Instantbird, and other XUL apps.

> Are you checking `ownerDocument` because that value is actually needed, or just as a check of "is a Document"?  It's possible for `document.ownerDocument` to be null, so it seems that would break this check, right?
> 
> Maybe instead check for the properties you need, such as `defaultView`?

Yeah, it's just to see if aArgsOrDocument is actually some kind of document. Good point about ownerDocument possibly being null. According to Neil in https://bugzilla.mozilla.org/show_bug.cgi?id=1025146#c46, I can use instanceof Document.

> I think this needs to be updated?  The highlighter menu item currently has no effect in partial mode.

Good eye, thanks!

> I think this needs to be updated?  The long line menu item currently has no effect in partial mode.

Well spotted, thanks.

> It looks like `viewPartialSource.xul` will now run this `onload`, as well as the one bound to `load` in `viewSource.js`.  Is that correct?

Yeah, that's correct. I think viewPartialSource.js could probably do with a refactor as well, since it's pretty tangle-y.

Having init and onXULLoaded run in viewSource.js should be fine. I'll update the code so that when we enter the \_loadViewSourceDeprecated method, we'll detect "selection" or "mathml" (the two supported values) in the lineNumber argument position, and just exit early. That way, viewSourcePartial.js can do its job properly.

Comment 48

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

https://reviewboard.mozilla.org/r/7403/#review6555

> Who are the users of the old API we are concerned with here?  Add-ons?

Add-ons, and other XUL apps like Thunderbird, SeaMonkey, Instantbird, etc.

> It seems like you forgot to finish this comment.

Forgot to remove it, actually. Good eye, thanks. :)

Comment 49

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

(In reply to [e-mail] from comment #46)
> Comment on attachment 8595559 [details]
> MozReview Request: bz://1025146/mconley
> 
> > +  // TODO: Is there a better way of determining whether or not aArgsOrDocument is
> > +  // in fact a document?
> instanceof Document (that might not work for CPOWs, but maybe you could just
> check for them first).
> 

This works wonderfully for both in-process documents and CPOWs. Thanks!

> > +    // We use the group message manager so that if we switch remoteness of the
> > +    // browser (which we might do if we're attempting to load the document
> > +    // source out of the network cache), we automatically re-inject the frame
> > +    // frame script.
> Out of interest, what was wrong with using the window message manager?

Purely copying the approach that we used in browser.js. However, I suspect it makes more sense in that context because of multiple types of browsers that we might not want to load the content scripts in.

I'll switch to using the window message manager - that seems to make more sense, thanks.

Comment 50

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

Comment on attachment 8595559 [details]
MozReview Request: bz://1025146/mconley

/r/7403 - Bug 1025146 - Modernize ViewSourceUtils to be able to use outerWindowIDs to load document sources. r=jryans.
/r/7405 - Bug 1025146 - Modernize viewSource.js to use a frame script. r=?
/r/7647 - Bug 1025146 - Update ViewSource tests to account for refactor. r=jryans.

Pull down these commits:

hg pull -r 75eb24ce880a653d6dfb98c72274573573ad04fc https://reviewboard-hg.mozilla.org/gecko/

Comment 51

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

Interdiff: https://reviewboard.mozilla.org/r/7405/diff/6-7/

Comment 52

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

https://reviewboard.mozilla.org/r/7405/#review6557

::: toolkit/components/viewsource/content/viewSource.js:74
(Diff revisions 6 - 7)
>      // We use the group message manager so that if we switch remoteness of the

Nit: Update this comment, since it's not group anymore.

Comment 53

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

Comment on attachment 8595559 [details]
MozReview Request: bz://1025146/mconley

https://reviewboard.mozilla.org/r/7401/#review6559

Ship It!

Comment 54

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

While testing bug 1158377, I noticed a few more issues with this work.

# View Source Title

For e10s windows, the view source window's title says "Source of: Nightly".  For non-e10s windows, it says "Source of: <URL>".

# Content Menu -> View Page Source fails

It reports:

Error: BrowserViewSourceOfDocument cannot accept a CPOW as a document. browser.js:13315:0

However, Cmd-U or Tools -> Web Developer -> Page Source both work.

The revision I tested here was from pulling bug 1158377 from MozReview and going back one changeset.

Comment 55

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

Comment on attachment 8595559 [details]
MozReview Request: bz://1025146/mconley

/r/8435 - Bug 1156493 - "e10s: move .cacheKey to nsICacheInfoChannel so child channels can get/set it" [r=michal.novotny]
/r/7403 - Bug 1025146 - Modernize ViewSourceUtils to be able to use outerWindowIDs to load document sources. r=jryans.
/r/7405 - Bug 1025146 - Modernize viewSource.js to use a frame script. r=jryans.
/r/8437 - Bug 1025146 - Make viewing the source of an iframe work with remote browsers. r=?
/r/7647 - Bug 1025146 - Update ViewSource tests to account for refactor. r=jryans.

Pull down these commits:

hg pull -r 3893427757f19707dbdd9068f9e6ba61d2269b88 https://reviewboard-hg.mozilla.org/gecko/

Comment 56

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

This latest set includes a fix for the context menu business.

The title is actually a bit harder to fix - correctly, anyhow. It also seems less critical, so I'll file a follow-up once this one closes.

Comment 57

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

Comment on attachment 8595559 [details]
MozReview Request: bz://1025146/mconley

https://reviewboard.mozilla.org/r/7401/#review7123

This does not fix the context menu case for me.

I think it's caused by [browser-context.inc][1], which needs to be updated to not use a CPOW.

[1]: https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser-context.inc#380

Comment 58

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

Ah, heh, yep - I misunderstood and fixed the (other broken) case for viewing source of iframes. I'll update the context menu entry as well, thanks.

Comment 59

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

Comment on attachment 8595559 [details]
MozReview Request: bz://1025146/mconley

/r/8435 - Bug 1156493 - "e10s: move .cacheKey to nsICacheInfoChannel so child channels can get/set it" [r=michal.novotny]
/r/7403 - Bug 1025146 - Modernize ViewSourceUtils to be able to use outerWindowIDs to load document sources. r=jryans.
/r/7405 - Bug 1025146 - Modernize viewSource.js to use a frame script. r=jryans.
/r/8437 - Bug 1025146 - Make viewing the source of a page or frame via the context menu work with remote browsers. r=?
/r/7647 - Bug 1025146 - Update ViewSource tests to account for refactor. r=jryans.

Pull down these commits:

hg pull -r aa21a0c4e49b00faf2a2000455442614817b3727 https://reviewboard-hg.mozilla.org/gecko/

Comment 60

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

Comment on attachment 8595559 [details]
MozReview Request: bz://1025146/mconley

https://reviewboard.mozilla.org/r/7401/#review7143

Great, it looks good to me!

Comment 61

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/5fe8e7a205e6
https://hg.mozilla.org/integration/fx-team/rev/13bf99d217bf
https://hg.mozilla.org/integration/fx-team/rev/28c41c53d0c2
https://hg.mozilla.org/integration/fx-team/rev/485ce3fb3fa8

Comment 62

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5fe8e7a205e6
https://hg.mozilla.org/mozilla-central/rev/13bf99d217bf
https://hg.mozilla.org/mozilla-central/rev/28c41c53d0c2
https://hg.mozilla.org/mozilla-central/rev/485ce3fb3fa8

Comment 63

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

Comment on attachment 8595559 [details]
MozReview Request: bz://1025146/mconley

Approval Request Comment
[Feature/regressing bug #]:

View Source. I believe the jryans and the DevTools team are hoping to move View Source into a tab sometime on 40, and it will need to build off of these patches.

[User impact if declined]:

View Source in a tab will likely not ship in 40. Users who tend to use View Source will hit the network for pages reached via POST requests if using e10s.

[Describe test coverage new/current, TreeHerder]:

Pretty extensive manual testing by jryans and myself, and there are automated tests for View Source that are all passing.

[Risks and why]: 

It's a complete refactor of some pretty battle-hardened code. It's possible there are some edge-cases here that we didn't foresee.

[String/UUID change made/needed]:

None.

Comment 64

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

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #63)
...
> View Source. I believe the jryans and the DevTools team are hoping to move
> View Source into a tab sometime on 40, and it will need to build off of
> these patches.
> 
> [User impact if declined]:
> 
> View Source in a tab will likely not ship in 40. Users who tend to use View
> Source will hit the network for pages reached via POST requests if using
> e10s.

This is correct - devtools needs this patch in Aurora in order to ship view-source-in-tab ( bug 1067325 ).

Comment 65

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

Note that the refactoring work and framescript is the real value for DevTools here. The actual bug that this bug describes for e10s and hitting the network is still going to affect 40 since bug 1156493 has been backed out. The refactoring should still be able to ride 40 though.

Comment 66

[Daniel Holbert [:dholbert]]

One of the commits in comment 61 added a deprecation warning that says:
{
You may find more details about this deprecation at: https://developer.mozilla.org/en-US/Add-ons/Code_snippets/ViewSource
}

However, this MDN page is 404. Is there a plan to get some content up there? (Or maybe the content already exists but the URL is wrong?)

Comment 67

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

Oh shoot - yeah, the URL is https://developer.mozilla.org/en-US/Add-ons/Code_snippets/View_Source_for_XUL_Applications.

I'll file a new bug for that.

Comment 68

[Sylvestre Ledru [:Sylvestre]]

> View Source. I believe the jryans and the DevTools team are hoping to move
> View Source into a tab sometime on 40, and it will need to build off of
> these patches.
Any reason why this cannot ride the train from 41?
The patch seems quite big to me

Comment 69

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

(In reply to Sylvestre Ledru [:sylvestre] from comment #68)
> > View Source. I believe the jryans and the DevTools team are hoping to move
> > View Source into a tab sometime on 40, and it will need to build off of
> > these patches.
> Any reason why this cannot ride the train from 41?
> The patch seems quite big to me

From a product perspective, the DevTools team was hoping to ship view source in a tab (bug 1067325) as part of the 40.1 spring release of Dev. Edition on June 2nd.  Assuming we get approval here, I'll soon also mark it for approval as well.

Bug 1067325 builds on top of the refactor in this bug, so this one would be needed for view source in tab to be able to land in Dev. Ed.

Comment 70

[Sylvestre Ledru [:Sylvestre]]

> From a product perspective, the DevTools team was hoping to ship view source in a tab (bug 1067325) as 
> part of the 40.1 spring release of Dev. Edition on June 2nd.
Is it a hard requirement for the spring release? I understand it is a cool new feature (myself, I wanted Firefox to do that for years) but both are big patches. Regressions are likely and June 2nd is just one week from now.

Comment 71

[Brian Grinstead [:bgrins]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #70)
> > From a product perspective, the DevTools team was hoping to ship view source in a tab (bug 1067325) as 
> > part of the 40.1 spring release of Dev. Edition on June 2nd.
> Is it a hard requirement for the spring release? I understand it is a cool
> new feature (myself, I wanted Firefox to do that for years) but both are big
> patches. Regressions are likely and June 2nd is just one week from now.

Forwarding that question to Jeff

Comment 72

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

(In reply to Brian Grinstead [:bgrins] from comment #71)
> (In reply to Sylvestre Ledru [:sylvestre] from comment #70)
> > > From a product perspective, the DevTools team was hoping to ship view source in a tab (bug 1067325) as 
> > > part of the 40.1 spring release of Dev. Edition on June 2nd.
> > Is it a hard requirement for the spring release? I understand it is a cool
> > new feature (myself, I wanted Firefox to do that for years) but both are big
> > patches. Regressions are likely and June 2nd is just one week from now.

After some discussion we agree - there is too much risk here to crash-land. Plus on the brighter side, we do need some unique features for 41!

Comment 73

[Sylvestre Ledru [:Sylvestre]]

Thanks guy. This is really appreciated.

Comment 76

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

Attachment: MozReview Request: Bug 1025146 - Modernize ViewSourceUtils to be able to use outerWindowIDs to load document sources. r=jryans.

Comment 77

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

Attachment: MozReview Request: Bug 1156493 - "e10s: move .cacheKey to nsICacheInfoChannel so child channels can get/set it" [r=michal.novotny]

Comment 78

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

Attachment: MozReview Request: Bug 1025146 - Make viewing the source of a page or frame via the context menu work with remote browsers. r=?

Comment 79

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

Attachment: MozReview Request: Bug 1025146 - Update ViewSource tests to account for refactor. r=jryans.

Comment 80

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

Attachment: MozReview Request: Bug 1025146 - Modernize viewSource.js to use a frame script. r=jryans.

Comment 81

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

Attachment: Bugnotes

http://people.mozilla.org/~mconley2/bugnotes/bug-1025146.html