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

VERIFIED FIXED in Firefox 41

Status

()

defect
P1
normal
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: alice0775, Assigned: jryans)

Tracking

(Blocks 1 bug, {regression, ux-trust})

41 Branch
mozilla43
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s-, firefox40 unaffected, firefox41+ verified, firefox42+ verified, firefox43 verified)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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
Flags: needinfo?(mconley)
Summary: external editor (view_source.editor.external) doesn't work on NIGHTLY 41.0a1 → [non-e10s]and[e10s] external editor (view_source.editor.external) doesn't work on NIGHTLY 41.0a1
Just like to add that setting the pref 'dom.ipc.processCount' to greater than one produces an empty edit window.
External editor (Firefox add-on) has ~12k users so we should consider fixing this for firefox21. Adding a tracking flag.
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?
Flags: needinfo?(mconley) → needinfo?(alice0775)
Flags: needinfo?(alice0775)
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.
Duplicate of this bug: 1176934
Duplicate of this bug: 1185963
Please backed out the offending bugs
Flags: needinfo?(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?
Flags: needinfo?(mconley) → needinfo?(jryans)
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
(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?
Flags: needinfo?(jryans) → needinfo?(jgriffiths)
(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.
Flags: needinfo?(jgriffiths)
Priority: -- → P1
Mike, would CPOWs be acceptable here, or are we trying to remove them entirely?
Flags: needinfo?(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...
Flags: needinfo?(mconley) → needinfo?(jryans)
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
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
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.
Assignee: nobody → jryans
Status: NEW → ASSIGNED
Flags: needinfo?(jryans)
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
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.
Any progress on this?
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]
(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.
Posted patch Patch1165599.patch (obsolete) — Splinter Review
I attached a small patch file to fix this issue.
Note that this patch is not tested well.
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.
Attachment #8658824 - Flags: review?(bgrinstead)
(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.
Attachment #8657408 - Attachment is obsolete: true
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.
Flags: needinfo?(jryans)
Florin, do you know by any chance whether using external editor to view source is working in 41.0b8 or not? Thanks.
Flags: needinfo?(florin.mezei)
(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.
Flags: needinfo?(jryans)
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
Attachment #8658824 - Flags: review+
Attachment #8658824 - Flags: review?(bgrinstead)
(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.
Attachment #8658824 - Attachment description: MozReview Request: Bug 1165599 - Restore basic external view source. r=bgrins → MozReview Request: Bug 1165599 - Restore basic external view source. r=jsantell
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 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.
Attachment #8658824 - Flags: approval-mozilla-beta?
Attachment #8658824 - Flags: approval-mozilla-aurora?
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.
Attachment #8658824 - Flags: approval-mozilla-beta?
Attachment #8658824 - Flags: approval-mozilla-beta+
Attachment #8658824 - Flags: approval-mozilla-aurora?
Attachment #8658824 - Flags: approval-mozilla-aurora+
Flags: qe-verify+
(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).
Flags: needinfo?(florin.mezei)
Blocks: 1203624
https://hg.mozilla.org/mozilla-central/rev/c638ef3e2f20
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Verified this issue as fixed on Windows 7 64-bit and Windows 10 64-bit using Firefox 41.0b9, buildID 20150910171927.
Status: RESOLVED → VERIFIED
QA Contact: cornel.ionce
Flags: qe-verify+
Blocks: 1190179
Blocks: 1204365
No longer depends on: 1204365
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).
You need to log in before you can comment on or make changes to this bug.