Fix fallout from bug 1418403: Remove viewing source in a stand-alone window (view source not working)

RESOLVED FIXED in Thunderbird 60.0

Status

RESOLVED FIXED
9 months ago
7 months ago

People

(Reporter: jorgk, Assigned: jorgk)

Tracking

(Blocks: 1 bug, {regression})

Trunk
Thunderbird 60.0
regression
Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird60+)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Assignee)

Description

9 months ago
Bug 1418403 has landed, so it's time to act.
(Assignee)

Comment 1

9 months ago
Related to bug 1163707, I suppose.

Comment 2

9 months ago
(In reply to Jorg K (GMT+1) from comment #1)
> Related to bug 1163707, I suppose.

I don't think so, we are migrated. The last users of deprecated APIs are in m-c.

Comment 3

9 months ago
In the current c-c, attempt to open View source produces this:

XML Parsing Error: undefined entity
Location: chrome://global/content/viewSource.xul
Line Number 17, Column 1:<window id="viewSource"
^
(Assignee)

Comment 4

9 months ago
Well, I see
XML Parsing Error: undefined entity
Location: jar:file:///C:/Temp/2018-02-17/thunderbird/omni.ja!/chrome/toolkit/content/global/netError.xhtml
Line Number 313, Column 41: netError.xhtml:313:41
(see bug 1439032)

and when clicking in the link to the file I get:
TypeError: window.top.openUILinkIn is not a function viewSourceUtils.js:73:5

So I suppose we could make the second one work by supplying that function.
(Assignee)

Comment 5

9 months ago
With bug 1439032 now fixed, when trying to view the source of a message, I get a window that says:
===
File not found
The file /C:/mozilla-source/comm-central/obj-x86_64-pc-mingw32/dist/bin/chrome/toolkit/content/global/viewSource.xul cannot be found. Please check the location and try again.
===

Clicking on a link in the Browser/Error Console, I now get:
TypeError: win.gBrowser is undefined viewSourceUtils.js:69:9
viewSource/onOpen/< chrome://global/content/viewSourceUtils.js:69:9

So it looks like we're dealing with two problems here. The first one can be fixed by forking viewSource.xul to C-C's common/ as suggested by FRG in a private message.

The second problem is the JS source view from the Browser/Error Console and here we need to mimic the browser sufficiently to get this to work since that's working in FF.
> So it looks like we're dealing with two problems here.

For 1: If we fork should the Should we fork with history intact? 

For 2: Might work in SeaMonkey out of the box but not sure if we have a selectedTab in which case some minor work in tabbrowser might be needed. For Thunderbird this probably needs a change in toolkit. Somewhat browser centric. Reminds me of the password prompting code in 51+ ...
(Assignee)

Comment 7

9 months ago
For 1: Before we fork, we should see what FF does and whether we can get the source view going in a tab.

For 2: Yes, it also reminds me of the password prompting code, but we also mimic browser here:
       https://hg.mozilla.org/comm-central/rev/f45975e98daa#l2.17
(Assignee)

Comment 8

9 months ago
I'm looking at a Firefox Nightly. View source opens in a tab, for example:
view-source:https://www.mozilla.org/de/firefox/60.0a1/whatsnew/?oldversion=58.0.2

And opening something from the Browser console gives:
view-source:resource://gre/modules/TelemetryStopwatch.jsm

Frankly, this is quite neat, it would be great if you could do something similar.
(Assignee)

Comment 9

9 months ago
Snooping around the M-C source, they use:
https://hg.mozilla.org/mozilla-central/rev/fcfdf000a8f3#l2.95
gViewSourceUtils.viewSourceInBrowser(args);
https://searchfox.org/mozilla-central/rev/cac28623a15ace458a8f4526e107a71db1519daf/toolkit/components/viewsource/content/viewSourceUtils.js#71

I wonder what would be required to use this for our message URLs.
(Assignee)

Updated

9 months ago
tracking-thunderbird60: --- → +
Keywords: regression
(Assignee)

Updated

9 months ago
Duplicate of this bug: 1439328
(Assignee)

Updated

9 months ago
Summary: Fix fallout from bug 1418403: Remove viewing source in a stand-alone window → Fix fallout from bug 1418403: Remove viewing source in a stand-alone window (view source not working)
(Assignee)

Updated

9 months ago
Duplicate of this bug: 1439539
(Assignee)

Comment 12

9 months ago
Created attachment 8954233 [details] [diff] [review]
1439021-view-source.patch - [landed comment #21]

This tries to restore stuff from
https://hg.mozilla.org/mozilla-central/rev/fcfdf000a8f3

Somehow I'm missing something. This shows the message source, but I get this error on the console:
JavaScript error: resource://gre/modules/ViewSourceBrowser.jsm, line 95: TypeError: WeakSet value must be an object, got null

And when exercising most menu item, I get:
JavaScript error: chrome://messenger/content/viewSource.xul, line 1: TypeError: viewSourceChrome is undefined

Richard: Is there something that I've obviously missed? - or -
Aceman: Can you take a look.

I wanted to fix the bustage, but this is not good enough :-(
Attachment #8954233 - Flags: feedback?(richard.marti)
Attachment #8954233 - Flags: feedback?(acelists)
Comment on attachment 8954233 [details] [diff] [review]
1439021-view-source.patch - [landed comment #21]

Thanks for looking into it.

When I revert, or remove this lines, it works: https://hg.mozilla.org/mozilla-central/diff/fcfdf000a8f3/toolkit/components/viewsource/ViewSourceBrowser.jsm#l1.33

The this.loadFrameScript(); shouldn't be executed.
Attachment #8954233 - Flags: feedback?(richard.marti) → feedback+
(Assignee)

Comment 14

9 months ago
Thanks for looking into it. So we're talking about this:

-    // If we have a known <browser> already, load the frame script here.  This
-    // is not true for the window case, as the element does not exist until the
-    // XUL document loads.  For that case, the frame script is loaded by
-    // viewSource.js.
-    if (this._browser) {
-      this.loadFrameScript();
-    }
+    // If we have a known <browser> already, load the frame script here.
+    this.loadFrameScript();

It will be hard to convince M-C to restore this. Well, they told us to fork what we need, and we need this snipped changed for the fork to work, so let's see.
(Assignee)

Comment 15

9 months ago
Created attachment 8954307 [details] [diff] [review]
1439021-M-C-part.patch

Brian, would you accept restoring the hunk which was removed here:
https://hg.mozilla.org/mozilla-central/rev/fcfdf000a8f3#l9.33

That way the forked viewSource.{xul|js|css|dtd|properties} would still work in TB for the moment.

The medium term plan is to get the "view source in tab" going for TB, but we're short in resources to investigate the feasibility and what would be involved to get that to work.

We also need to fix the problem that clicking on a source file in the developer console doesn't work.
Attachment #8954307 - Flags: review?(bgrinstead)
Comment on attachment 8954307 [details] [diff] [review]
1439021-M-C-part.patch

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

Seems OK to me, but forwarding review to jryans
Attachment #8954307 - Flags: review?(bgrinstead) → review?(jryans)
Comment on attachment 8954307 [details] [diff] [review]
1439021-M-C-part.patch

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

Seems reasonable to me, thanks for working on it! :)

::: toolkit/components/viewsource/ViewSourceBrowser.jsm
@@ +75,5 @@
>  
> +    // If we have a known <browser> already, load the frame script here. This
> +    // is not true for the window case (still used by other applications like
> +    // Thunderbird), as the element does not exist until the XUL document loads.
> +    if (this._browser) {

I would suggest moving these details into `loadFrameScript`, and have it return early when there's no `browser`.  Also, this file seems to use the getter version throughout (`this.browser`), so seems best to follow that trend.
Attachment #8954307 - Flags: review?(jryans) → review+
(Assignee)

Comment 18

9 months ago
Created attachment 8954518 [details] [diff] [review]
1439021-M-C-part.patch (v2) [landed comment #29]

Thanks for the quick review. Here's a patch that incorporates your suggestions.

I removed the original comment
  If we have a known <browser> already, load the frame script here.
since it appeared out of place in front of an unconditional call.
Attachment #8954307 - Attachment is obsolete: true
Attachment #8954518 - Flags: review?(jryans)
Comment on attachment 8954518 [details] [diff] [review]
1439021-M-C-part.patch (v2) [landed comment #29]

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

Thanks, looks great to me! :)
Attachment #8954518 - Flags: review?(jryans) → review+
(Assignee)

Comment 20

9 months ago
Thanks for the quick turn-around.

Dear sheriff, could you please land "1439021-M-C-part.patch (v2)" on inbound.
Keywords: checkin-needed
(Assignee)

Updated

9 months ago
Keywords: leave-open

Comment 21

9 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/2caecab6e8c0
restore 'view source' by forking M-C code. r=me (fork-onky)
Keywords: checkin-needed
(Assignee)

Comment 22

9 months ago
Dear sheriff, could you please land "1439021-M-C-part.patch (v2)" on inbound.
Flags: needinfo?(aryx.bugmail)
Flags: needinfo?(apavel)
Keywords: checkin-needed
(Assignee)

Comment 23

9 months ago
Comment on attachment 8954233 [details] [diff] [review]
1439021-view-source.patch - [landed comment #21]

So this was no WIP after all thanks to Richard's suggestion. This already shows the message source again and will work even better with the M-C part landed as well :-)
Attachment #8954233 - Attachment description: 1439021-view-source.patch - WIP (v1) → 1439021-view-source.patch - [landed comment #21]
Attachment #8954233 - Flags: feedback?(acelists)

Comment 24

9 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/172a4b9b4974
Follow-up: remove import-globals-from linter directives. rs=bustage-fix DONTBUILD
Keywords: checkin-needed

Comment 25

9 months ago
Pushed by aciure@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6188bb22f81e
Check for browser in loadFrameScript() to support Thunderbird. r=jryans
Jorg, the patch has been landed.
Flags: needinfo?(aryx.bugmail)
Flags: needinfo?(apavel)
The edit menu in the source view will be affected by bug 1439766 when it lands. I can look at it this evening.
(Assignee)

Comment 28

9 months ago
Thanks. The M-C part hasn't been merged, so it will be broken before it ever worked :-(
(Assignee)

Comment 29

9 months ago
Oops, ignore that, it has been merged:
https://hg.mozilla.org/mozilla-central/rev/6188bb22f81e
Check for browser in loadFrameScript() to support Thunderbird. r=jryans

Comment 31

9 months ago
Comment on attachment 8954233 [details] [diff] [review]
1439021-view-source.patch - [landed comment #21]

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

OK, if this is restoration of the original code from m-c, then probably nothing to review here.

But clicking on errors in the console doesn't open the source file, with a message like:
Error: An error was thrown inside one of your components, but React doesn't know what it was. This is likely due to browser flakiness. React does its best to preserve the "Pause on exceptions" behavior of the DevTools, which requires some DEV-mode only tricks. It's possible that these don't work in your browser. Try triggering the error in production mode, or switching to a modern browser. If you suspect that this is actually an issue with React, please file an issue. 1 react-dom-dev.js:624:19

And I also sometimes get:
TypeError: this.gViewSourceUtils is undefined 1, hudservice.js:400:5

gViewSourceUtils is not mentioned in this patch. Do we need to somehow provide it too, similar to gViewSourceBundle ?
Attachment #8954233 - Flags: review+
(Assignee)

Comment 32

9 months ago
(In reply to :aceman from comment #31)
> OK, if this is restoration of the original code from m-c, then probably
> nothing to review here.
Thanks. The restored code only ever worked for message source, not JS source. I made a mistake that Richard corrected for me later: https://hg.mozilla.org/comm-central/rev/5e07cff9cd9c422239f82edc33e2aec5f00f45b7#l2.51

> But clicking on errors in the console doesn't open the source file, with a
> message like:
> Error: An error was thrown inside one of your components, but React doesn't
> know what it was. This is likely due to browser flakiness. React does its
> best to preserve the "Pause on exceptions" behavior of the DevTools, which
> requires some DEV-mode only tricks. It's possible that these don't work in
> your browser. Try triggering the error in production mode, or switching to a
> modern browser. If you suspect that this is actually an issue with React,
> please file an issue. 1 react-dom-dev.js:624:19
Oh, I saw that with pref view_source.editor.external set to true. Try with false. It's all here:
https://searchfox.org/mozilla-central/source/toolkit/components/viewsource/content/viewSourceUtils.js#48

Comment 33

9 months ago
Created attachment 8955821 [details] [diff] [review]
load viewSourceUtils.js [landed comment #35]

This makes gViewSourceUtils defined.
Clicking a source link in the Error console still fails with win.gBrowser not being defined at viewSourceUtils.js:69.

But, setting view_source.editor.external=true and view_source.editor.path to your preferred viewer/editor makes the source file of the error open in that editor. Could be a temporary measure for devs :)

There is still "aCallBack is not a function 1 viewSourceUtils.js:268" output, but I file that as a separate bug as that seems to be a real bug in m-c.
Attachment #8955821 - Flags: review?(jorgk)
(Assignee)

Comment 34

9 months ago
Comment on attachment 8955821 [details] [diff] [review]
load viewSourceUtils.js [landed comment #35]

Yes, that works. For the record, I set view_source.editor.path to
C:\Program Files (x86)\Notepad++\notepad++.exe
Yes, with the backslashes and the spaces and it still works ;-)
Attachment #8955821 - Flags: review?(jorgk) → review+

Comment 35

9 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/248a595bf6bc
load viewSourceUtils.js into Thunderbird to make 'view source' work for more cases. r=jorgk
(Assignee)

Comment 36

9 months ago
Hmm, I see
JavaScript error: chrome://messenger/content/mailCore.js, line 20: ReferenceError: XPCOMUtils is not defined
Flags: needinfo?(acelists)

Comment 37

8 months ago
Created attachment 8956971 [details] [diff] [review]
import XPCOMUtils [landed comment #40]

Yes, I see it too now. I wander why I didn't see it before.
Strangely it still works (makes the View source open) even with that error, which I would expect to not load gViewSourceUtils and thus do nothing.

So this should solve the error.
Flags: needinfo?(acelists)
Attachment #8956971 - Flags: review?(jorgk)
(Assignee)

Comment 38

8 months ago
Comment on attachment 8956971 [details] [diff] [review]
import XPCOMUtils [landed comment #40]

Looks OK, I assume you tried it ;-)
Attachment #8956971 - Flags: review?(jorgk) → review+
(Assignee)

Updated

8 months ago
Keywords: checkin-needed

Comment 39

8 months ago
I did, but so did I the previous patch :)

Comment 40

8 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/b5ddfa0a002c
Follow-up: import missing XPCOMUtils in mailCore.js. r=jorgk
Keywords: checkin-needed
(Assignee)

Comment 41

8 months ago
Aceman, the trick with the external browser still works in TB 60 beta, but not in Daily any more. Can you take a look?

Also see bug 1448689.
Flags: needinfo?(acelists)
(Assignee)

Comment 42

8 months ago
Hmm, works in a local build, but not in Daily 61.0a1 (2018-03-26) (64-bit).
(Assignee)

Comment 43

8 months ago
Filed bug 1450205 for that.
Flags: needinfo?(acelists)
Blocks: 1451081
(Assignee)

Updated

7 months ago
Blocks: 1454041
(Assignee)

Updated

7 months ago
Attachment #8954518 - Attachment description: 1439021-M-C-part.patch (v2) → 1439021-M-C-part.patch (v2) [landed comment #29]
(Assignee)

Updated

7 months ago
Attachment #8955821 - Attachment description: load viewSourceUtils.js → load viewSourceUtils.js [landed comment #35]
(Assignee)

Updated

7 months ago
Attachment #8956971 - Attachment description: import XPCOMUtils → import XPCOMUtils [landed comment #40]
(Assignee)

Comment 44

7 months ago
In this bug here we forked "view (message) source" for mail messages and added a kludge so "view (JS/XML/etc) source" from the error/browser console will work if view_source.editor.external and view_source.editor.path are set.

That worked for a while, at least in TB 60, and then went broken again, at least in server builds, see bug 1450205.

Since this bug here has become too long and twisted, I'm moving the remaining work over to bug 1454041.

All patches here landed on version 60 of C-C and M-C, no uplifts.
Assignee: nobody → jorgk
Status: NEW → RESOLVED
Last Resolved: 7 months ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 60.0
You need to log in before you can comment on or make changes to this bug.