993847 - Move event handling for SeaMonkey's aboutSyncTabs.xul page into aboutSyncTabs.js

Status: RESOLVED FIXED

(SeaMonkey :: Sync UI, defect)

Description

[rsx11m]

+++ This bug was initially created as a clone of Bug #960566 +++

> With the current plan to harden the security of Firefox, we want to
> disallow internal privileged pages to use inline JavaScript.

Similar to bug 993845 on the session restore page, this is a privileged page and has some inline JavaScript for onxxx="..." events, but written in XUL. This page also has a parallel aboutSyncTabs.js file already which needs to be extended.

Comment 1

[rsx11m]

I think I can come up with a patch for this, that's fairly mechanical, but I don't know how to test it given that I don't have a sync server set up anywhere. Thus, it'll be checked for syntax errors only.

Comment 2

[rsx11m]

Attachment: Mostly untested patch

This removes all inline event-handler attributes from the XUL codes, adds ids to the respective elements, and then re-adds those as event listeners in the init() function of the JavaScript code.

Initially I had an explicit window.onload function adding the RemoteTabViewer.* event listeners, but then figured that I could equally well just put them into the init() definition. If this doesn't work I can post the earlier patch.

I don't see any syntax errors when running this, nor anything in the Error Console, thus with a bit of luck this works out of the box with sync properly set up.

Comment 3

[rsx11m]

Attachment: Easier-to-read patch

I was a bit too lazy here layout-wise; let's move the id attributes to the top of the element definitions where they are usually expected. No other changes.

Comment 4

[Jens Hatlak (:InvisibleSmiley)]

Hmm, got an error while testing. Seems we need to port bug 841096 part 1 (mozilla-central changeset aa32872101fe). I'll need to do that at least locally before testing your patch.

Comment 5

[Jens Hatlak (:InvisibleSmiley)]

(In reply to Jens Hatlak (:InvisibleSmiley) from comment #4)
> Hmm, got an error while testing. Seems we need to port bug 841096 part 1
> (mozilla-central changeset aa32872101fe).

Filed bug 998466 for that.

Comment 6

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 8407968 [details] [diff] [review]
Easier-to-read patch

With the current patch from bug 998466 applied, about:sync-tabs works. With yours on top I get:

Error: NS_ERROR_XPC_BAD_CONVERT_JS: Could not convert JavaScript argument arg 0 [nsIObserverService.addObserver]
Source File: chrome://communicator/content/aboutSyncTabs.js
Line: 16

The error goes away with the following, but please ask Neil or similar for confirmation whether this fix is correct please:

-window.onload = RemoteTabViewer.init;
-window.onunload = RemoteTabViewer.uninit;
+window.onload = function() { RemoteTabViewer.init(); }
+window.onunload = function() { RemoteTabViewer.uninit(); }

But then, when right-clicking an entry from the remote tabs list, I still get:

Error: TypeError: this._tabsList is undefined
Source File: chrome://communicator/content/aboutSyncTabs.js
Line: 196

Please have a look.

Comment 7

[rsx11m]

Attachment: Comments addressed, I think (v3a)

Hmm, directly using the functions as the event handlers didn't throw anything for me, but as said, I didn't see it doing anything useful without sync having been set up either way.

I saw the "this._tabsList is undefined" before in a different context, and it was due to the order of definitions. I've now put "this.buildList(true);" before adding the event listeners, this may make the difference.

This patch goes on top of bug 998466 attachment 8409288 [details] [diff] [review]. Adding Neil for feedback as suggested.

Comment 8

[rsx11m]

Attachment: Alternative patch (v3b)

(In reply to rsx11m from comment #2)
> Initially I had an explicit window.onload function adding the
> RemoteTabViewer.* event listeners, but then figured that I could equally
> well just put them into the init() definition.

This is the other patch, rebased on top of bug 998466 attachment 8409288 [details] [diff] [review]. Thus, if attachment 8409329 [details] [diff] [review] doesn't work, please give this one a try.

Comment 9

[[e-mail]]

(In reply to rsx11m from comment #8)
> Created attachment 8409330 [details] [diff] [review]

I don't think this one is going to work.

Comment 10

[[e-mail]]

Comment on attachment 8409329 [details] [diff] [review]
Comments addressed, I think (v3a)

Actually this one has the same problem.

When you add a function as an event listener it gets called with the element as the this object, not any object on which it may have been set as a property.

Comment 11

[[e-mail]]

The neatest way I can see of fixing this is to add a handleEvent function to RemoteTabViewer. It could check the event type, calling init, uninit, handleClick or adjustContextMenu as appropriate, except for the command event, where it would need to check the target's id to determine what to do.

You would then add the RemoteTabViewer object itself as the event listener in all the places where you're currently trying to add one of its methods, although for the context menu you could simplify it by adding the event listener to the menupopup instead of each of the menuitems individually.

Comment 12

[Jens Hatlak (:InvisibleSmiley)]

As Neil guessed, neither patch works (I still get the undefined error).

Now that the dependency has been fixed, testing should be a bit easier. Why not set up Sync? It's not that hard, and once you have at least one open tab synced from another client (can be another SM profile/session), you don't need the other client anymore (as far as this bug is concerned).

BTW feel free to remove the trailing whitespace on the context="tabListContext" line while you're here.

Comment 13

[rsx11m]

Guess I'd have to figure out first how exactly the event handling works here as my understanding is apparently less than insufficient. So, this is obviously not as straight-forward as I thought... :-\

Fortunately bug 923920 isn't imminent.

Comment 14

[rsx11m]

I'm unable to set up a sync account with either 2.28a1 or 2.27a2 builds. I get an "Unknown error" in the Account Details page selecting "SeaMonkey Sync Server" and following in the Error Console (trunk):

No chrome package registered for chrome://browser/locale/sync.properties

Timestamp: 4/20/2014 12:25:44 PM                (this comes right after typing in the e-mail address)
Error: Unknown response from server: <html>
 <head>
  <title>404 Not Found</title>
 </head>
 <body>
  <h1>404 Not Found</h1>
  The resource could not be found.<br /><br />
 </body>
</html>
Source File: resource://services-sync/userapi.js
Line: 143

1398014765110 Sync.BrowserIDManager ERROR basicPassword getter should be not used in BrowserIDManager
1398014774620 Sync.BrowserIDManager ERROR basicPassword getter should be not used in BrowserIDManager

The [Next] button remains disabled even after entering the password.

Comment 15

[rsx11m]

I've created the Sync account with a Firefox release build (which worked flawlessly) and then tried to connect SeaMonkey trunk by pairing with that 3-group/4-character key. This gave me the following:

Error: [Exception... "basicPassword setter should be not used in BrowserIDManager'basicPassword setter should be not used in BrowserIDManager' when calling method: [nsIRunnable::run]"  nsresult: "0x8057001e (NS_ERROR_XPC_JS_THREW_STRING)"  location: "native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0"  data: no]

Comment 16

[rsx11m]

Since the device pairing works with 2.25 this looks like a recent regression, thus filed bug 998807. Nevertheless, while with 2.25 I could establish the account I still see an empty tab list.

Is there any description/tutorial how this is supposed to work?

Comment 17

[Jens Hatlak (:InvisibleSmiley)]

(In reply to rsx11m from comment #16)
> Is there any description/tutorial how this is supposed to work?

Set up Sync in two browsers/sessions. Open a tab in one and trigger a Sync run (via Tools, Sync Now). Open about:sync-tabs in the other and wait 2-3 sec for it to update (or Sync Now manually).

Comment 18

[rsx11m]

Ah, I see - now I've got it and finally have those tabs listed with a "Sync Now" issued. Thanks.

Comment 19

[rsx11m]

(In reply to [e-mail] from comment #10)
> When you add a function as an event listener it gets called with the element
> as the this object, not any object on which it may have been set as a property.

This was the key information, also explaining why

(In reply to Jens Hatlak (:InvisibleSmiley) from comment #6)
> -window.onload = RemoteTabViewer.init;
> -window.onunload = RemoteTabViewer.uninit;
> +window.onload = function() { RemoteTabViewer.init(); }
> +window.onunload = function() { RemoteTabViewer.uninit(); }

was necessary. Based on attachment 8409330 [details] [diff] [review] (v3b) I've wrapped all the directly passed event handler functions into function() { RemoteTabViewer.*(); } and is works now, the reason being that the scope of the "this" is RemoteTabViewer again and not the Element object where the event was caught.

Following Neil's comment #11 on adding the event listener to the menupopup instead of each of the menuitems individually, I'll have a look how to optimize this in order to reduce the number of listeners. I likely will have to stick with window.load and window.unload definitions, and I don't see how to combine the two tabsList events, but consolidating the five menuitem listeners certainly would be an advantage already.

Comment 20

[rsx11m]

Attachment: Patch with common menuitem handlers (v4b)

This is attachment 8409330 [details] [diff] [review] with calling the handlers resolved for the "this" ambiguity and the "command" handlers consolidated in a single event handler attached to "tabListContext" instead of individual menuitems.

I also took care of the trailing white space as suggested.

Comment 21

[rsx11m]

Attachment: Patch with two handler functions only (v4c)

(In reply to [e-mail] from comment #11)
> The neatest way I can see of fixing this is to add a handleEvent function to
> RemoteTabViewer. It could check the event type, calling init, uninit,
> handleClick or adjustContextMenu as appropriate, except for the command
> event, where it would need to check the target's id to determine what to do.
> 
> You would then add the RemoteTabViewer object itself as the event listener
> in all the places where you're currently trying to add one of its methods,

The v4c patch is trying to implement this in a common EventHandler object, but not quite relying on RemoteTabViewer alone. In fact, trying this I ran into the problem again of handling self-references within the RemoteTabViewer object. Similarly, this object itself can't handle the "load" and "unload" events given that again it would require a self-reference to set up the handlers.

So, the compromise I came up with are to define two handler functions, one for window.on{load,unload} which sets up all events based on a single EventDirector handler. This in turn invokes the individual RemoteTabViewer methods based on the event type and id of the event target. RemoteTabViewer itself remains unchanged in this way.

Please pick the variant you like better (I'd say v4c = this one).

Comment 22

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 8409948 [details] [diff] [review]
Patch with two handler functions only (v4c)

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

I also prefer this version, but would be OK with any of the two if both work (only tested this one). The comments I made here mostly apply to both patches.

r=me with the issues addressed.

::: suite/common/sync/aboutSyncTabs.js
@@ +244,5 @@
>    }
>  };
> +
> +let EventDirector = {
> +  handleEvent: function(event) {

I think for the Firefox code (which most of the SeaMonkey Sync code originates from) there's some kind of agreement that larger functions should be named. Don't know what for exactly (debuggability?), but we might want to not leave that road.

@@ +245,5 @@
>  };
> +
> +let EventDirector = {
> +  handleEvent: function(event) {
> +    switch(event.type) {

switch is not a function, it's a control flow statement (or whatever it's actually called). Hence please have a space between it and the following opening bracket. [I think this is part of some style guide we follow; if not, it certainly is part of mine.]

@@ +253,5 @@
> +      case "contextmenu":
> +        RemoteTabViewer.adjustContextMenu(event);
> +        break;
> +      case "command":
> +        switch(event.target.id) {

See "switch" above.

@@ +260,5 @@
> +            break;
> +          case "bookmarkSingleTab":
> +            RemoteTabViewer.bookmarkSingleTab();
> +            break;
> +          case "openSelected":

Hehe, didn't it make you wonder why openSelected is listed twice here? The second one is never going to be executed.

@@ +278,5 @@
> +    }
> +  }
> +};
> +
> +window.onload = window.onunload = function(event) {

Hmm, I don't quite like having both loaders in one function. Sure it works, but what's the point? It needlessly "hides" the unload code at the bottom of the function body.

Again, we might want to have larger functions named (see discussion above).

@@ +279,5 @@
> +  }
> +};
> +
> +window.onload = window.onunload = function(event) {
> +  switch(event.type) {

See "switch" above.

::: suite/common/sync/aboutSyncTabs.xul
@@ +35,2 @@
>                  showFor="single"/>
> +      <menuitem id="openSelected"

Duplicate element ID.

Comment 23

[rsx11m]

Attachment: Patch for check-in (v5c)

(In reply to Jens Hatlak (:InvisibleSmiley) from comment #22)
> I also prefer this version, but would be OK with any of the two if both work
> (only tested this one). The comments I made here mostly apply to both
> patches.

This is based on attachment 8409948 [details] [diff] [review] (v4c).

> I think for the Firefox code (which most of the SeaMonkey Sync code
> originates from) there's some kind of agreement that larger functions should
> be named. Don't know what for exactly (debuggability?), but we might want to
> not leave that road.

Ok, it appears that the convention is roughly to combine objectname_methodname() as function name, thus I've followed that for the new functions.

> switch is not a function, it's a control flow statement (or whatever it's
> actually called). Hence please have a space between it and the following
> opening bracket. [I think this is part of some style guide we follow; if
> not, it certainly is part of mine.]

Done in all instances.

> Hehe, didn't it make you wonder why openSelected is listed twice here? The
> second one is never going to be executed.

Oops, guess I was editing a bit too mechanically here. :-D

I've renamed them to "openSingleTab" and "openSelectedTabs" to match the ids of the bookmark items.

> Hmm, I don't quite like having both loaders in one function. Sure it works,
> but what's the point? It needlessly "hides" the unload code at the bottom of
> the function body.

Split again into separate functions, named as window_onload() and window_onunload(),

> > +window.onload = window.onunload = function(event) {
> > +  switch(event.type) {

thus the switch () is no longer needed.

> ::: suite/common/sync/aboutSyncTabs.xul
> @@ +35,2 @@
> >                  showFor="single"/>
> > +      <menuitem id="openSelected"
> 
> Duplicate element ID.

Resolved.

BTW: I observed an issue with the context menu not being updated after all tabs have been opened and hence disappeared from the list. I've filed bug 999780 on this as it happens with or without the patch here, thus an independent issue.

Comment 24

[rsx11m]

Push for comm-central, please.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/3235e26c17c8

Comment 26

[Philip Chee]

> +  handleEvent: function EventDirector_handleEvent(event) {
...........................^^^^^^^^^^^^^^^^^^^^^^^^^

Sorry for the drive-by but there is now no need to decorate methods with e.g. "EventDirector_handleEvent". The new JSD2 debugger has been improved enough that it can now identify where the anonymous function properly belongs. So:

handleEvent: function (event) {...};

HTH.

Comment 27

[rsx11m]

Attachment: Follow-up patch

Ok, so if it's no longer needed then let's remove it. I find that double naming rather distracting [and also bloats the code by a significant few bytes ;-) ].

Comment 28

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 8412281 [details] [diff] [review]
Follow-up patch

Well, if it makes you happy...

Comment 29

[rsx11m]

:-)

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/4ab1373c5f88

Comment 31

[Jens Hatlak (:InvisibleSmiley)]

(In reply to rsx11m from comment #14)
> No chrome package registered for chrome://browser/locale/sync.properties

I guess this is due to:
http://mxr.mozilla.org/comm-central/source/mozilla/services/sync/modules/engines/clients.js#118

I wonder whether you see this error message at all, given that the offending statement is wrapped in a try/catch. Do you happen to have caught exceptions being written to the Error Console enabled (dom.report_all_js_exceptions I think)?

If this was actually an issue, looking at where the "move c-c to m-c" discussion went, I think it'd be useless trying to fix the source. Instead we should just make that chrome URL available (through jar.mn I guess).

> Source File: resource://services-sync/userapi.js
> Line: 143

This is in function _onUsername when the response body is neither 0 nor 1.