1165599 - [non-e10s]and[e10s] external editor (view_source.editor.external) doesn't work on NIGHTLY 41.0a1

Status: VERIFIED FIXED

(Toolkit :: View Source, defect, P1)

Description

[Alice0775 White]

It has broken due to landing 1025146.

The same thing in the past is happening many times:(
Are there no automated test?

Regression window:
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=b4018ab42fd3&tochange=2529d411d5b1

REGRESSED BY: Bug 1025146

Comment 1

[Gary [:streetwolf52]]

Just like to add that setting the pref 'dom.ipc.processCount' to greater than one produces an empty edit window.

Comment 2

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

External editor (Firefox add-on) has ~12k users so we should consider fixing this for firefox21. Adding a tracking flag.

Comment 3

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

Gary - that's bug 1165309 (though I should point out that dom.ipc.processCount > 1 is not currently a supported configuration).

Alice0775 - I'm able to get the external editor opening on my Windows machine on Nightly... can you give me some STR?

Comment 4

[Alice0775 White]

Build Identifier:
https://hg.mozilla.org/mozilla-central/rev/d44425c6730c
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:41.0) Gecko/20100101 Firefox/41.0 ID:20150524030234

Reproducible: always, 100%

Steps to reproduce:
1. Create New Profile
2. Create user.js and append the followings
user_pref("view_source.editor.external", true);
user_pref("view_source.editor.path", "C:\\windows\\system32\\notepad.exe");
3. Open Nightly41.0a1
4. Open any webpage
5. Right click on empty area of page
6. Choose "View Page Source"

That is all.

Actual Results:
Buildt-in viewsource window opened

On the other hand,
Open Aurora40.0a2 with the same profile and follow STR from Step4.
it works as expected.

Expected Results:
notepad.exe should launch and the page source is in it.


So, this is bug of Nightly41.0a1.

Comment 7

[Alice0775 White]

Please backed out the offending bugs

Comment 8

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 42.

Comment 9

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

It looks like the viewSourceUtils.js code that handles external view source editors needs to be modified in order to handle the new multi-process reality.

I don't think that's going to be a small task, unfortunately. I'm also uncertain if this is something we want to continue to support, as part of "great or dead".

What do you think, jryans?

Comment 10

[Helge Skjeveland]

Mike, I have E10s turned on. As of 42.0a1, an alternate View Source editor works fine for me if I set Notepad2 as that by modifying FFs about:config (Boolean & path) to invoke it for editing source while using David Ficano's fine ViewSourceWith addon, and have its Advanced tab's "Use in place of native message/page source menu item" box checked. So far, it's completely transparent.

Helge Skjeveland, bachware.com

Comment 11

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

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #9)
> It looks like the viewSourceUtils.js code that handles external view source
> editors needs to be modified in order to handle the new multi-process
> reality.
> 
> I don't think that's going to be a small task, unfortunately. I'm also
> uncertain if this is something we want to continue to support, as part of
> "great or dead".
> 
> What do you think, jryans?

I think a few small tweaks could get it working again, if we were willing to live with CPOW usage here (not sure if that's acceptable or not).  I agree that a proper conversion for e10s is a larger task.

I don't view this feature as something we must have built in to the browser.  Leaving it for an add-on to provide may be the right way to go here.

Jeff, do you have any input on the priority of this view source in an external editor feature?

Comment 12

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

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #11)
> (In reply to Mike Conley (:mconley) - Needinfo me! from comment #9)
> > It looks like the viewSourceUtils.js code that handles external view source
> > editors needs to be modified in order to handle the new multi-process
> > reality.
> > 
> > I don't think that's going to be a small task, unfortunately. I'm also
> > uncertain if this is something we want to continue to support, as part of
> > "great or dead".
> > 
> > What do you think, jryans?
> 
> I think a few small tweaks could get it working again, if we were willing to
> live with CPOW usage here (not sure if that's acceptable or not).  I agree
> that a proper conversion for e10s is a larger task.
> 
> I don't view this feature as something we must have built in to the browser.
> Leaving it for an add-on to provide may be the right way to go here.
> 
> Jeff, do you have any input on the priority of this view source in an
> external editor feature?

I wish I had a better idea of how many users have this pref set. I *think* it's small? It's certainly hard enough to find and finicky enough to get set up properly.

I would prefer to do this:

* fix it for now via small tweaks and CPOWS usage, if that's feasible
* measure usage and values of these two prefs in the meantime, in nightly, DE and Beta: "view_source.editor.external", "view_source.editor.path"

Ryan, Mike: if the lighter-weight CPOWS approach isn't working we should discuss again  - probably on a mailing list.

Comment 13

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

Mike, would CPOWs be acceptable here, or are we trying to remove them entirely?

Comment 14

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

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #13)
> Mike, would CPOWs be acceptable here, or are we trying to remove them
> entirely?

A CPOW would be fine as a band-aid solution, but they'll be going away eventually.

So yeah, I think they're appropriate for use here.

Are you able to take this on? I'm afraid I'm about to go on some long-duration PTO...

Comment 15

[D Merrick]

Just for clarity, I'm assuming the external view source is to view the original source file (wanted), rather than the generated html (although that could also secondarily be useful).
If (from the thread) it's easier as an add-on, would it not be quicker to write a basic add-on and automatically migrate or advise people with the preference onto the add-on.
d

Comment 16

[Helge Skjeveland]

Again, I have E10s turned on, and now, as of 43.0a1 on Win7SP1, an alternate View Source editor still works fine for me (I set Notepad2 as that by modifying FFs about:config's Boolean & path) to invoke for editing source, while using David Ficano's fine ViewSourceWith addon, and its Advanced tab's "Use in place of native message/page source menu item" box is checked. So far, it's completely transparent. I've been using NP2 for editing HTML that way via FFx64 for weeks.

Helge Skjeveland, bachware.com

Comment 17

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

I'll take a look here soon.  As Helge says in comment 16, it seems there's already an add-on that can be used if needed.

Comment 18

[D Merrick]

An existing add-on sounds promising to me. I'll take a look at it (as soon as the bug's fixed (filed) that's making my add-on list blank!)
Cheers, david

Comment 19

[cat_in_136]

https://hg.mozilla.org/releases/mozilla-beta/file/be23fd6c2114/browser/base/content/browser.js#l2376
https://hg.mozilla.org/releases/mozilla-beta/file/be23fd6c2114/toolkit/components/viewsource/content/viewSourceUtils.js#l56

In chrome://global/content/viewSourceUtils.js, gViewSourceUtils.viewSource() calls openInExternalEditor() in case of view_source.editor.external=true and _openInInternalViewer() in case of view_source.editor.external=false.
The 1st argument of _openInInternalViewer() is aArgsOrURL but one of openInExternalEditor() is still aURL.
So openInExternalEditor() fails to open an editor and fall back to an internal editor, I think.

Comment 20

[Gary [:streetwolf52]]

Any progress on this?

Comment 21

[Toby Boyd]

I am on DE 42.0a2 (2015-09-02) on OS X 10.9.5, and I was hoping the ViewSourceWith extension might fix this for me, but I get CPOW errors when it tries to invoke Sublime Text:

viewPage: [Exception... "It's illegal to pass a CPOW to native code arg 7 [nsIWebBrowserPersist.saveURI]"  nsresult: "0x80570036 (NS_ERROR_XPC_CANT_PASS_CPOW_TO_NATIVE)"  location: "JS frame :: chrome://viewsourcewith/content/urldownloader.js :: ViewSourceWithUrlDownloader.prototype.internalSaveURI :: line 134"  data: no]

Comment 22

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

(In reply to Toby Boyd from comment #21)
> I am on DE 42.0a2 (2015-09-02) on OS X 10.9.5, and I was hoping the
> ViewSourceWith extension might fix this for me, but I get CPOW errors when
> it tries to invoke Sublime Text:
> 
> viewPage: [Exception... "It's illegal to pass a CPOW to native code arg 7
> [nsIWebBrowserPersist.saveURI]"  nsresult: "0x80570036
> (NS_ERROR_XPC_CANT_PASS_CPOW_TO_NATIVE)"  location: "JS frame ::
> chrome://viewsourcewith/content/urldownloader.js ::
> ViewSourceWithUrlDownloader.prototype.internalSaveURI :: line 134"  data: no]

As suggested in #16, you need multiprocess support turned off for this to work.

Comment 23

[cat_in_136]

Attachment: Patch1165599.patch

I attached a small patch file to fix this issue.
Note that this patch is not tested well.

Comment 24

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

Attachment: MozReview Request: Bug 1165599 - Restore basic external view source. r=jsantell

Bug 1165599 - Restore basic external view source. r=bgrins

This restores external view source after changes in bug 1025146.  It does
function in e10s, however the page descriptor is not used, so viewing a POST
result will GET the page instead.  This is the same as it was before bug
1025146.

Follow ups will add usage tracking and improve e10s behavior if there is enough
usage.

Comment 25

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

(In reply to cat_in_136 from comment #23)
> Created attachment 8657408 [details] [diff] [review]
> Patch1165599.patch
> 
> I attached a small patch file to fix this issue.
> Note that this patch is not tested well.

Thanks for posting this!  I've done something similar in mine and will continue to evolve it.

Comment 26

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

Ryan, is there a chance we can get a fix for this landed soon into Beta41? It's not ideal to ship with this regression.

Comment 27

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

Florin, do you know by any chance whether using external editor to view source is working in 41.0b8 or not? Thanks.

Comment 28

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

(In reply to Ritu Kothari (:ritu) from comment #26)
> Ryan, is there a chance we can get a fix for this landed soon into Beta41?
> It's not ideal to ship with this regression.

The patch I've posted today should take care of it.  I'll request uplift assuming reviews go well.

Comment 29

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8658824 [details]
MozReview Request: Bug 1165599 - Restore basic external view source. r=jsantell

https://reviewboard.mozilla.org/r/18707/#review16739

Looks good! No comments from me other than some nits below, not sure how review permissions work in either of these components though.

::: browser/base/content/browser.js:2402
(Diff revision 1)
> +    return;

Instead of early returning, might be clearer just to do an \`else\` here just to call \`viewInternal\`

::: toolkit/components/viewsource/content/viewSourceUtils.js:223
(Diff revision 1)
> +      Deprecated.warning("The arguments you're passing to viewSource.xul " +

So happy we're minimizing the huge arguments length for these functions

::: toolkit/components/viewsource/content/viewSourceUtils.js:419
(Diff revision 1)
> +      if (this.contentLoaded) {

Is this so onContentLoaded is called only once? Add a quick comment for this

Comment 30

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

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #29)
> Comment on attachment 8658824 [details]
> MozReview Request: Bug 1165599 - Restore basic external view source. r=bgrins
> 
> https://reviewboard.mozilla.org/r/18707/#review16739
> 
> Looks good! No comments from me other than some nits below, not sure how
> review permissions work in either of these components though.

I believe your review is enough here, I've previously reviewed in this area for mconley.

Comment 31

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

Comment on attachment 8658824 [details]
MozReview Request: Bug 1165599 - Restore basic external view source. r=jsantell

Bug 1165599 - Restore basic external view source. r=jsantell

This restores external view source after changes in bug 1025146.  It does
function in e10s, however the page descriptor is not used, so viewing a POST
result will GET the page instead.  This is the same as it was before bug
1025146.

Follow ups will add usage tracking and improve e10s behavior if there is enough
usage.

Comment 32

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

Comment on attachment 8658824 [details]
MozReview Request: Bug 1165599 - Restore basic external view source. r=jsantell

Approval Request Comment
[Feature/regressing bug #]: Bug 1025146
[User impact if declined]: External view source will fail, appears in a window instead.
[Describe test coverage new/current, TreeHerder]: No new tests, but tested manually.
[Risks and why]: Low, requires non-default pref to enable.
[String/UUID change made/needed]: None.

Comment 33

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/c638ef3e2f20

Comment 34

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

Comment on attachment 8658824 [details]
MozReview Request: Bug 1165599 - Restore basic external view source. r=jsantell

Approved as this pref is off by default so the risk is minimal.

Comment 35

[Cornel Ionce [:noni] [Hubs QA]]

(In reply to Ritu Kothari (:ritu) from comment #27)
> Florin, do you know by any chance whether using external editor to view
> source is working in 41.0b8 or not? Thanks.

I've tested today and using external editor to view source is not working on Firefox 41 beta 8 (buildID 20150907144446).

Comment 36

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

https://hg.mozilla.org/releases/mozilla-beta/rev/deac8c4a8367
https://hg.mozilla.org/releases/mozilla-aurora/rev/2598a159ed18

Comment 37

[Phil Ringnalda (:philor)]

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

Comment 38

[Cornel Ionce [:noni] [Hubs QA]]

Verified this issue as fixed on Windows 7 64-bit and Windows 10 64-bit using Firefox 41.0b9, buildID 20150910171927.

Comment 39

[Cornel Ionce [:noni] [Hubs QA]]

Confirming the fix on Windows 7 64-bit and Windows 10 64-bit using:
*Firefox 42, buildID 20151029151421/
*Firefox 43 RC build 1 (buildID 20151208100201).