1060529 - e10s - commands not updated properly

Status: VERIFIED FIXED

(Core :: XUL, defect)

Description

[Neil Deakin]

The enabled state of UI isn't updated when the child process needs it.

Steps:

1. click on a web page.
2. open the edit menu

Actual: all of clipboard commands are enabled, as well as Undo
Expected: none of those commands are enabled

The problem here is that nsGlobalWindow::UpdateCommands needs to go up to the parent to send its commands from the child process.

There will likely be some some more complexity when trying to get the focused element which the parent only sees as a <browser/> element when it really wants the child element (an <input> on a page for example).

Comment 2

[:Felipe Gomes (needinfo for replies!)]

See http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/RemoteController.jsm#23

Comment 4

[Chris Peterson [:cpeterson]]

This bug should be fixed for the e10s M2 milestone.

Comment 5

[Marco Mucci [:MarcoM]]

Hi Neil, can you assign a point value and qe-verification.

Comment 6

[Neil Deakin]

Attachment: updatecommandsredirect

This is working but needs some polish. It has this code flow:

1. When a commandupdate occurs, the child calls the parent to get a list of commands to update, that is, a list of commands the UI has to update. The command updater syntax is changed a bit to allow for a list to be specified.
2. The child takes this list and determines which commands are enabled and disabled using the controllers.
3. The enabled/disabled list is sent to the parent to update its UI.

This might be a bit slower on non-Mac where this currently doesn't happen right away, and instead happens when opening the menu. I'm going to continue making some improvements.

Comment 8

[Chris Peterson [:cpeterson]]

Neil: is your "updatecommandsredirect" patch ready preliminary review? The e10s team is trying to tie up the loose ends on our last few e10s bugs for our M2 milestone this week. Can we defer this bug to our next milestone ("M3" in 1–2 weeks)? This bug is annoying but doesn't seem like a blocker for dogfood testing.

Comment 9

[Neil Deakin]

Attachment: updatecommandsredirect

Some improvements over the last patch. Don't fire the commandupdate event. Doing so improves performance significantly.

Comment 10

[Neil Deakin]

Attachment: Part 2 - remove the lazy command updating

Part 1 should have made this faster. We can try and see.

Comment 11

[[e-mail]]

Comment on attachment 8488819 [details] [diff] [review]
updatecommandsredirect

>+    sync EnableDisableCommands(nsCString[] enableCommands,
>+                               nsCString[] disableCommands);
Does this need to be sync?

>+      uint32_t count;
>+      char** commands;
>+      xulCommandDispatcher->GetCommandsToUpdate(aAction, &count, &commands);
>+
>+      for (uint32_t c = 0; c < count; c++) {
>+        aCommands->AppendElement(commands[c]);
You're leaking commands[c] and commands itself. Better would be to make everying an nsTArray<nsCString>& throughout (c.f. nsDocumentViewer::AppendSubtree).

Comment 12

[[e-mail]]

Comment on attachment 8488819 [details] [diff] [review]
updatecommandsredirect

>+    // This will retrieve from the parent browser a list of commands that
>+    // need to be updated when this action occurs.
>+    nsTArray<nsCString> commands;
>+    mTabChild->GetCommandsToUpdate(mAction, &commands);
>+
>+    nsTArray<nsCString> enableCommands, disableCommands;
>+
>+    // Iterate over the commands and enable and disable them as necessary.
Also, how do we know that the remote window has focus?

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8488819 [details] [diff] [review]
updatecommandsredirect

>-[scriptable, uuid(f3c50361-14fe-11d3-bf87-00105a1b0627)]
>+[scriptable, uuid(95344B98-FDDE-412D-AB1B-4F7F45BDB15E)]
> interface nsIDOMXULCommandDispatcher : nsISupports
Couldn't you make this builtinclass

> {
>-            attribute nsIDOMElement    focusedElement;
>-            attribute nsIDOMWindow     focusedWindow;
>+  attribute nsIDOMElement    focusedElement;
>+  attribute nsIDOMWindow     focusedWindow;
> 
>   void                      addCommandUpdater(in nsIDOMElement updater,
>                                               in DOMString events,
>                                               in DOMString targets);
>   void                      removeCommandUpdater(in nsIDOMElement updater);
> 
>   void                      updateCommands(in DOMString eventName);
> 
>   nsIController             getControllerForCommand(in string command);
>   nsIControllers            getControllers();
> 
>   void advanceFocus();
>   void rewindFocus();
>   void advanceFocusIntoSubtree(in nsIDOMElement elt);
>   attribute boolean suppressFocusScroll;
>+
>+  void                      getCommandsToUpdate(in DOMString eventName,
>+                                                out unsigned long count,
>+                                                [retval, array, size_is(count)] out string commands);
And then make this noscript or such and use nsTArray<nsString> or nsTArray<nsCString>

>+
>+    /**
>+     * Indicate, based on the current state, that some commands are enabled and
>+     * some are disabled.
>+     */
>+    sync EnableDisableCommands(nsCString[] enableCommands,
>+                               nsCString[] disableCommands);
If possible, keep stuff async


>+TabParent::RecvGetCommandsToUpdate(const nsString& aAction,
>+                                   nsTArray<nsCString>* aCommands)
>+{
>+  nsCOMPtr<nsIContent> frame = do_QueryInterface(mFrameElement);
>+  if (!frame)
>+    return true;
mFrameElement is an nsIContent, so no need to QI.
Also, always {} with if. (I know TabParent and TabChild aren't quite consistent, but since they aren't, better to use Mozilla coding style)

>+TabParent::RecvEnableDisableCommands(const nsTArray<nsCString>& aEnableCommands,
>+                                     const nsTArray<nsCString>& aDisableCommands)
>+{
>+  nsCOMPtr<nsIContent> frame = do_QueryInterface(mFrameElement);
>+  if (!frame)
>+    return true;
Similar stuff here.

>+
>+  nsIDocument* document = frame->OwnerDoc();
>+
>+  for (uint32_t c = 0; c < aEnableCommands.Length(); c++) {
>+    Element* element = document->GetElementById(NS_ConvertUTF8toUTF16(aEnableCommands[c]));
>+    if (element) {
>+      element->UnsetAttr(kNameSpaceID_None, nsGkAtoms::disabled, true);
>+    }
Given that UnsetAttr may dispatch MutationEvents, you must keep element alive.
So, nsCOMPtr<Element>, not Element*


>+  }
>+
>+  for (uint32_t c = 0; c < aDisableCommands.Length(); c++) {
>+    Element* element = document->GetElementById(NS_ConvertUTF8toUTF16(aDisableCommands[c]));
>+    if (element) {
>+      element->SetAttr(kNameSpaceID_None, nsGkAtoms::disabled, NS_LITERAL_STRING("true"), true);
ditto.


Hmm, we don't check any of the elements is XUL or anything like that. Should we? Note, document can be normal HTML document in
case of b2g.

Comment 14

[[e-mail]]

(In reply to Olli Pettay from comment #13)
> Hmm, we don't check any of the elements is XUL or anything like that. Should
> we? Note, document can be normal HTML document in case of b2g.

The commandset is a XUL element, which suggests that the rest will be too?

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

TabParent::RecvEnableDisableCommands just uses getElementById.

Comment 16

[Neil Deakin]

Attachment: Update commands, v2

Addresses the issues described above. I added an IsXUL check but it probably isn't necessary.

I made EnableDisableCommands async but it will probably need to be sync instead for additional work I'm doing to fix some context menu commands.

Comment 17

[Neil Deakin]

Attachment: Part 3 - don't treat a raise of the child process window specially

The change is part 1 made it so that command updating doesn't occur when the child window isn't focused. However, normally commands don't update when windows are raised. This isn't desired as it will mean that switching tabs doesn't update commands properly due to the aWindowRaised flag passed to nsFocusManager::Focus.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8490097 [details] [diff] [review]
Part 3 - don't treat a raise of the child process window specially

>@@ -2019,17 +2019,17 @@ nsFocusManager::RaiseWindow(nsPIDOMWindo
>   if (!aWindow || aWindow == mActiveWindow || aWindow == mWindowBeingLowered)
>     return;
> 
>   if (sTestMode) {
>     // In test mode, emulate the existing window being lowered and the new
>     // window being raised.
>     if (mActiveWindow)
>       WindowLowered(mActiveWindow);
>-    WindowRaised(aWindow);
>+    WindowRaised(aWindow, false);
This looks wrong. aWindow is a "child" in child process - or at least it is a 
"child" when it is the topmost window object in the child process or inside embedding.


>@@ -211,19 +211,21 @@ interface nsIFocusManager : nsISupports
>   /** move focus to the root element in the document */
>   const unsigned long MOVEFOCUS_ROOT = 7;
>   /** move focus to a link at the position of the caret. This is a special value used to
>    *  focus links as the caret moves over them in caret browsing mode.
>    */
>   const unsigned long MOVEFOCUS_CARET = 8;
> 
>   /**
>-   * Called when a window has been raised.
>+   * Called when a window has been raised. if aIsChild is true then the
>+   * window is expected to be a child window, either in the embedded case
>+   * or a child process. If false, the window is a real native window.
Odd comment. Window is never native window. However it can be the topmost window
(in the parent process).


Those fixed, r+.

This is a bit error prone though.
Other option would be to add a flag to the outer window to indicate it is a top most "child window"

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8490043 [details] [diff] [review]
Update commands, v2


(Enn left IRC before I managed to comment...)
So I would prefer using commandupdate, and certainly not do
Set/UnsetAttr in two different places.

Where do you see event firing being slow? What is taking the time?
Certainly not event dispatch in this kind of rarely happening case.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

If you see slowness, could you please upload the profile.
Gecko profiler should work quite well with Nightlies (make sure to not use pseudo stacks).

Comment 21

[Neil Deakin]

Attachment: Part 1 - Update commands, v3

This version goes back to using an event to update commands. TabParent::RecvEnableDisableCommands still sets the attributes. Changing that would require some other mechanism such as creating a custom event object to supply the command list.

I used the profiling as suggested above and this version is a bit slower, '14' compared to '10'.

Comment 22

[Neil Deakin]

Attachment: Part 3 - don't treat a raise of the child process window specially

Not sure if there's some edge case I'm not aware of, but this handles the child process case.

Comment 23

[Neil Deakin]

Comment on attachment 8488822 [details] [diff] [review]
Part 2 - remove the lazy command updating

Oops, wrong patch

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8490972 [details] [diff] [review]
Part 3 - don't treat a raise of the child process window specially

s/NULL/nullptr/

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Neil Deakin from comment #21)
> I used the profiling as suggested above and this version is a bit slower,
> '14' compared to '10'.
What do those numbers mean?

Do you have a link to a cleopatra profile?

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8490969 [details] [diff] [review]
Part 1 - Update commands, v3

It is still total mystery to me why we need the new Set/UnsetAttr stuff.
What is wrong with events? Why the need for adding new complexity?

> This version goes back to using an event to update commands.
> TabParent::RecvEnableDisableCommands still sets the attributes. Changing
> that would require some other mechanism such as creating a custom event
> object to supply the command list.
Why don't you just handle command state changes like it is done in the
parent process. commandupdate event is dispatched.

Maybe I'm missing something obvious here, but really need some clarification
why the attribute approach is taken.

Comment 27

[Neil Deakin]

> Why don't you just handle command state changes like it is done in the
> parent process. commandupdate event is dispatched.

As indicated, that would require creating a custom event to hold the enabled/disabled state of the various commands, which the event listener would then need to retrieve, iterate over, and set the attributes. I could do that, but that in my mind seems more complicated, not less.

Comment 28

[Olli Pettay [:smaug][[e-mail]]]

Hmm, a new event interface for this would be trivial.

But if we use the Set/UnsetAttr approach, we sure should do it always, not only in e10s case.
And the attribute value should be set somewhere in nsXULCommandDispatcher.
TabParent really shouldn't know about this kind of xul specific attribute stuff.

But let me look at CommandDispatcher some more...

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

[19:00]	smaug	Enn: why can't we just attach a controller to the remote <xul:browser>. The state of the commands would be updated from child process. WindowRoot seems to take the controllers from focused element, and xul:browser has focus when the web content has focus.
[19:00]	smaug	that should, as far as I see, keep the setup simpler and closer to what it is now
[19:00]	smaug	feel free to disagree

Comment 30

[Neil Deakin]

Attachment: Part 1 - Update commands, v4

Olli suggested calling the controller with the list of enabled/disabled commands and caching them there, instead of setting the attributes directly in TabParent. I modified this suggestion slightly to instead add an interface to the remote browser element that can be called from TabParent to do this.

Comment 31

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8491671 [details] [diff] [review]
Part 1 - Update commands, v4

The patch is missing the new .idl file, but based on the code it looks pretty
much what I expect.

Don't you leak enableCommands and disableCommands ?
Couldn't EnableDisableCommands take arrays of CStrings, not char** ?


It is not clear to me why we need commands attribute in commandset.

Comment 32

[Neil Deakin]

> Don't you leak enableCommands and disableCommands ?
> Couldn't EnableDisableCommands take arrays of CStrings, not char** ?

I'm not aware of a syntax for declaring this.


> It is not clear to me why we need commands attribute in commandset.

So that the parent process can provide the child process with a list of commands that need updating, depending on which type of update needs to occur.

Comment 33

[Olli Pettay [:smaug][[e-mail]]]

How many commands do we have in general? Could we not update all the commands related to the current
state. Like, when an input element is focused, child process would access
nsWindowRoot::GetControllers and that would give the controllers for the element, and we'd
update all those command states to the parent side.

Comment 34

[Neil Deakin]

(In reply to Olli Pettay [:smaug] from comment #33)
> How many commands do we have in general?

A content-editable area has over 50 commands (bold, italic, increase font size, etc) We'll need to add them to RemoteController.jsm:supportsCommand to get them to work but that can be a later bug.

Comment 35

[Neil Deakin]

Attachment: Update commands, v5

Didn't get any more feedback, so let's try this

Comment 36

[Achwaq Khalid]

The "copy link location" is also absent! should i file a bug for it too or is it a known/related issue?

Comment 37

[Neil Deakin]

Copy Link is bug 1058883.

Comment 38

[Achwaq Khalid]

(In reply to Neil Deakin from comment #37)
> Copy Link is bug 1058883.

Oh my bad! the funny thing is i've seen it before =D

Comment 39

[Olli Pettay [:smaug][[e-mail]]]

I'm still not quite happy with the approach. It feels odd that we need to change commandset syntax and behavior for this.
Would it not be possible to send a list of enabled commands from child to parent when needed and 
the rest would be disabled?

Comment 40

[Olli Pettay [:smaug][[e-mail]]]

NeilAway, do you perhaps have strong opinions on this?

Comment 41

[Olli Pettay [:smaug][[e-mail]]]

Enn, is there really no way to forward the state of the commands from child to parent?
I'd prefer not to special case commandset for e10s.

Comment 42

[Neil Deakin]

The intent here is that the child doesn't know what commands the parent needs to update. Please let me know if you have some other idea. I personally dislike the existing commandset syntax.

Comment 43

[Olli Pettay [:smaug][[e-mail]]]

Well, if there are just few enabled command in the child, why not send them all to the parent?

Comment 44

[Neil Deakin]

As indicated earlier, there are more than 50 commands for content-editable areas, however almost none of these are used in the default UI. The current code prevents most commands from updating unless UI for them is visible (by default none are updated); this was added for performance. The commands instead update when a menu is opened. This isn't feasible here as we can't ask the child process for this state when a menu is opened. Instead, the child asks the parent which commands it has in its UI and menus. This is done by placing the attributes on the commandset elements. Thus, the child only needs to determine the state for those commands. In the future, this could be extended to allow the child controllers to determine the state of a set of commands at once, rather than individually.

Comment 45

[Olli Pettay [:smaug][[e-mail]]]

But how many of those 50 commands are enabled at the same time normally?
And the state of 50 commands can be packed to 50 bits if really needed.

Comment 46

[Neil Deakin]

I'm not following what you're asking here. The commands are enabled and disabled individually as the controllers currently only have a way to ask for the enabled state of one command at a time.

Comment 47

[Olli Pettay [:smaug][[e-mail]]]

Yeah, looks like controllers would need to have a way to enumerate all the controller objects and 
ask for supported commands. 
Making that work would be IMO less ugly than requiring using commands attribute in order to make
command e10s compatible.

Is there some particular reason why you prefer having commands attribute?

Comment 48

[Neil Deakin]

I'm not clear what you're proposing.

Comment 49

[Olli Pettay [:smaug][[e-mail]]]

Instead of asking parent process about the commands, child would send state information about
all the relevant commands. For that it would iterate through all the Controller objects in Controllers and ask which commands they support. No synchronous messaging from child to parent and back, only async update from child to parent.

Comment 50

[Olli Pettay [:smaug][[e-mail]]]

Still wondering, is there some particular reason why you'd prefer having commands attribute?
Am I missing something here?

Comment 51

[Neil Deakin]

(In reply to Olli Pettay [:smaug] from comment #49)
> Instead of asking parent process about the commands, child would send state
> information about
> all the relevant commands.

I'm not clear why you think we should spend extra time computing the state of many commands that aren't used in the default UI. Or why we should do compute the state of over 50 commands when only one needs to be updated.

I also think that changing the controller interfaces is more obtrusive than just adding a commands attribute which doesn't require any compatibility changes.

Comment 52

[Olli Pettay [:smaug][[e-mail]]]

sync messaging should be avoided if somewhat easily possible. It can cause child process to wait for a long-ish time doing nothing.

Adding an attribute the way the patch does requires one to explicitly use it in order to make 
commands work in e10s. Doesn't the patch break by default all the addons using commands for web pages?
(I agree that adding a new method to nsIController may also break stuff)

Comment 53

[[e-mail]]

(In reply to Olli Pettay from comment #49)
> Instead of asking parent process about the commands, child would send state
> information about
> all the relevant commands. For that it would iterate through all the
> Controller objects in Controllers and ask which commands they support. No
> synchronous messaging from child to parent and back, only async update from
> child to parent.
There's no requirement for the focused controller to support the command that the UI wants to update. For instance, you might be moving focus from a textarea to a link, in which case you need to disable the Paste menuitem (because a link doesn't know what pasting is).

(In reply to Olli Pettay from comment #50)
> Still wondering, is there some particular reason why you'd prefer having
> commands attribute?

The traditional command update method works like this.
An interesting event happens (let's say, some text was selected).
The command dispatcher is notified about the event, and looks for listening commandupdaters.
The commandupdaters then run arbitrary script which usually calls goUpdateCommand a few times.
(Each call to goUpdateCommand retrieves the state from the controller and updates the element.)

Naturally this is unworkable in e10s since the state retrieval operation would have to be a synchronous call to the child process, so instead whenever an interesting event happens in the current e10s tab we want to forward all the command states that the UI is interested in so that it can cache them and/or directly update its commands.

The problem here is that the traditional command update mechanism doesn't expose the list of interested commands to update (the nearest I could find was in SeaMonkey's main mail window where the list of commands to update is simply the child nodes of the command updater).

(In reply to Olli Pettay from comment #52)
> sync messaging should be avoided if somewhat easily possible.

Possibility:
Browser element notifies the child process of the events and commands it's interested in (possibly linking the commands to the relevant events, but that may be over-engineering it).
When an event occurs in the child process, the child process grabs the current state of the commands and notifies the browser element.
When the browser receives the notification, it updates its internal command state (simple if the child always sends the complete state) and then redispatches the event to the window if it has focus.
Normal commandupdate then takes over in the usual way.

Comment 54

[Neil Deakin]

> sync messaging should be avoided if somewhat easily possible. It can cause child
> process to wait for a long-ish time doing nothing.

We'll need to make it synchronous anyway for context menu commands such as copy image that can't update until the mouse button is pressed.


> Possibility:
> Browser element notifies the child process of the events and commands it's
> interested in (possibly linking the commands to the relevant events, but
> that may be over-engineering it).

I considered this option, essentially sending the list of desired commands to the child tags when the tab opens. I'm concerned that this will be fragile in the case where the tab focuses something quickly. Another option is to have the child ask for these commands during initialization, for example during the Browser:Init message which is already sent synchronously. And possibly add some mechansim to update the list if it changes, although I'm not sure if there's a case where this happens.

Regardless, the parent would still need to have some means of knowing which commands to tell the child it is interested in. I used the commands attribute for this.

Comment 55

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Neil Deakin from comment #54)
> > sync messaging should be avoided if somewhat easily possible. It can cause child
> > process to wait for a long-ish time doing nothing.
> 
> We'll need to make it synchronous anyway for context menu commands such as
> copy image that can't update until the mouse button is pressed.
> 
Don't see how this is related. We'd show the context menu once we've got the event+data from the child process.
(I think we even have code for this from the time when <html:menuitem> was implemented)

Comment 56

[Olli Pettay [:smaug][[e-mail]]]

If we take the commands attribute approach, can we at least use that in all the cases, also
in non-e10s? I really don't want inconsistent handling for e10s vs. non-e10s.

Comment 57

[Olli Pettay [:smaug][[e-mail]]]

Possibly in such way that there is a warning in browser console if commandset has commandupdater="true" but doesn't have commands attribute.
(and just having commands would be probably ok, it would implicitly mean commandupdater="true")

Comment 58

[Olli Pettay [:smaug][[e-mail]]]

...but we'd support commandset without commands attribute for some time, so that addons can be updated.

(Though, I still think making nsIController to tell which commands it supports would be nicer approach, but I can live with commands attribute assuming we don't do anything e10s specific with it.)

Comment 59

[Neil Deakin]

This patch does use the commands attribute in both cases. The one case is ChildCommandDispatcher::Run where it gets the list of commands, iterates over them and determines the enabled state. The other case is done in globalOverlay.js::goUpdateCommandUpdater called via the commandupdate event, which more or less does the same thing. Existing commandupdaters (such as in Thunderbird or addons) should continue to work unchanged, although addons would need to add a commands attribute if they want to work in all situations.

I do think that changing the nsIController interface would be a viable option, but it will require some more thought to avoid performing necessary work. I have some ideas here that I think would be better in a followup bug.

Would it be ok if we used this patch and I'll file a bug about changing nsIController?

Comment 60

[Olli Pettay [:smaug][[e-mail]]]

Could you at least add a warning (shown in browser console or somewhere) if commandset has commandupdater attribute but no commands attribute. Warning about using deprecated attribute or so.

Hmm, if we have commands attribute setup, is there a reason to change nsIController?
Don't we want one or the other, but not both?

commands attribute + warning in case commandset is there without commands attribute is fine to me, 
and so is changing nsIController.

Comment 61

[Olli Pettay [:smaug][[e-mail]]]

Or perhaps the warning should say that "use of commandupdater attribute only is deprecated, please add
also commands attribute" or something like that.

Comment 62

[Neil Deakin]

(In reply to Olli Pettay [:smaug] from comment #60)
> Could you at least add a warning (shown in browser console or somewhere) if
> commandset has commandupdater attribute but no commands attribute. Warning
> about using deprecated attribute or so.
> 

I wasn't intending that the existing style be deprecated. That would seem to defeat the purpose of sending the event. But it's true that you wouldn't want to use it when dealing with commands from a child content tab. Other cases though, such as the places window/downloads list or the command updaters in Thunderbird would continue working unchanged.

Comment 63

[Olli Pettay [:smaug][[e-mail]]]

I don't want there to be different syntax for e10s-enabled commandsets and other commandsets.
Inconsistency in APIs is guaranteed to lead to errors.

We need to change the API here (in one way or another),
but if we can support both the old and new syntax for some time and warn about use of the old one, 
that is close to the best option I can imagine.

Comment 64

[Neil Deakin]

I don't agree, but I'm hoping Neil can review this before I make any more changes.

Comment 65

[Olli Pettay [:smaug][[e-mail]]]

You don't agree consistency being important? Or what do you not agree?

Comment 66

[Neil Deakin]

I don't think the existing syntax should be considered the 'old' way. Both ways are still supported.

Comment 67

[Olli Pettay [:smaug][[e-mail]]]

The issue is that having two syntaxes one is forced to think whether the commandset might be used with
some remote browser. So whoever adds a commandset element needs to know all the contexts where the
element might be used. Sounds rather bad from maintenance point of view.

Comment 68

[(no longer active)]

Comment on attachment 8493834 [details] [diff] [review]
Update commands, v5

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

I thought about this today.  I think it's clear that none of the alternatives here are great.  That being said, I actually think modifying nsIController is the right thing to do here.  According to <https://mxr.mozilla.org/addons/search?string=nsIController&find=&findi=&filter=\W%5BNn%5DsIController\W&hitlimit=&tree=addons>, there are only 19 add-ons (if I'm counting correctly) that implement nsIController.  Since those are JS implementations, I think the effects of them not supporting the new method on nsIController isn't actually disastrous, since we can just treat a failure result from that method as an indication that the controller doesn't know how to handle the new command update model, and just have such nsIControllers not update properly with e10s.

Depending on how much we care about add-on compat with e10s, we can also get in touch with those add-on authors.

Comment 69

[Neil Deakin]

Attachment: Update commands, changing nsIController interface

OK, so this patch changes the nsIController interface instead. Another option is to change the nsICommandController interface as that is also implemented by the ones relevant to content use and would cause less compatibility issues with other users.

This patch works in most cases, but there are a few bugs to work out still.

Comment 70

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8512968 [details] [diff] [review]
Update commands, changing nsIController interface

>+
>+  void getSupportedCommands(out unsigned long count,
>+                            [array, size_is(count), retval] out string commands);
indentation look a bit odd when comparing to the existing methods.

Does
(out unsigned long count, [array, size_is (count), retval] out ACString commands)
not work?


>+  NS_IMETHOD Run()
>+  {
>+    nsCOMPtr<nsPIWindowRoot> root = mWindow->GetTopWindowRoot();
>+    if (!root)
>+      return NS_OK;
always {} with 'if'
Same also elsewhere


>+    nsTArray<nsCString> enabledCommands, disabledCommands;
>+    root->GetEnabledCommands(enabledCommands, disabledCommands);
>+    if (enabledCommands.Length() || disabledCommands.Length()) {
>+      mTabChild->EnableDisableCommands(enabledCommands, disabledCommands);
>+    }
So, curious, how large these arrays are.


>+    }
>+    else {
} else {
Same also elsewhere


>+      nsPIDOMWindow *rootWindow = nsGlobalWindow::GetPrivateRoot();
nsPIDOMWindow* 
Similar possibly also elsewhere.

>+nsWindowRoot::AddEnabledCommands(nsIControllers* aControllers,
>+                                 nsTHashtable<nsCharPtrHashKey>& aCommandsHandled,
>+                                 nsTArray<nsCString>& aEnabledCommands,
>+                                 nsTArray<nsCString>& aDisabledCommands)
Surprising method name.
Isn't this more like GetEnabledDisabledCommands()

>+nsWindowRoot::GetEnabledCommands(nsTArray<nsCString>& aEnabledCommands,
>+                                 nsTArray<nsCString>& aDisabledCommands)
>+{
>+  nsTHashtable<nsCharPtrHashKey> commandsHandled;
>+
>+  nsCOMPtr<nsIControllers> controllers;
>+  GetControllers(getter_AddRefs(controllers));
>+  if (controllers) {
>+    AddEnabledCommands(controllers, commandsHandled,
>+                       aEnabledCommands, aDisabledCommands);
>+  }
...it is after all called here.



>+  enableDisableCommands: function(aEnabledLength, aEnabledCommands,
>+                                  aDisabledLength, aDisabledCommands)
>+  {
>+    let doc = this._browser.ownerDocument;
>+
>+    // We could call window.updateCommands to update the disabled state
>+    // of the <command> elements, but this shortcut used here is faster.
>+    for (let c = 0; c < aEnabledLength; c++) {
>+      let command = aEnabledCommands[c];
>+      this._supportedCommands[command] = true;
>+      let commandElement = doc.getElementById(command);
>+      if (commandElement) {
>+        commandElement.removeAttribute("disabled");
>+      }
>+    }
>+
>+    for (let c = 0; c < aDisabledLength; c++) {
>+      let command = aDisabledCommands[c];
>+      this._supportedCommands[command] = false;
>+      let commandElement = doc.getElementById(command);
>+      if (commandElement) {
>+        commandElement.setAttribute("disabled", "true");
>+      }
>+    }
>+  }
Why we need all this? Why is isCommandEnabled not enough

Comment 71

[Neil Deakin]

> Does
> (out unsigned long count, [array, size_is (count), retval] out ACString
> commands)
> not work?

No. It gets declared as 'nsACString & *commands'


> >+    nsTArray<nsCString> enabledCommands, disabledCommands;
> >+    root->GetEnabledCommands(enabledCommands, disabledCommands);
> >+    if (enabledCommands.Length() || disabledCommands.Length()) {
> >+      mTabChild->EnableDisableCommands(enabledCommands, disabledCommands);
> >+    }
> So, curious, how large these arrays are.

Around 50-60 commands all together.

Comment 72

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Neil Deakin from comment #71)
> No. It gets declared as 'nsACString & *commands'
is that an issue?
commands[0] = somevalue; doesn't work?

Comment 73

[Neil Deakin]

No, it gets declared as a pointer to a reference and doesn't compile:

 0:01.91 ../../dist/include/nsIController.h:43:65: error: 'commands' declared as a pointer to a reference of type 'nsACString_internal &'
 0:01.91   NS_IMETHOD GetSupportedCommands(uint32_t *count, nsACString & *commands) = 0;

Comment 74

[Neil Deakin]

Attachment: Update commands, changing nsIController interface, v1.2

Now includes a test.

Comment 75

[(no longer active)]

Comment on attachment 8514348 [details] [diff] [review]
Update commands, changing nsIController interface, v1.2

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

Mostly a bunch of minor issues, but this seems very close.

::: dom/base/nsGlobalWindow.cpp
@@ +9330,5 @@
> +
> +    return NS_OK;
> +  }
> +
> +  nsRefPtr<nsGlobalWindow>             mWindow;

Nit: private

@@ +9388,5 @@
> +  }
> +
> +  // XXXndeakin Bug 1091187 - I'm not really clear why this code was added here
> +  // when it doesn't do anything related to updating commands.
> +  if (gSelectionCaretPrefEnabled && mDoc) {

We're moving this code out in bug 1090008.

::: dom/base/nsPIWindowRoot.h
@@ +32,5 @@
>    virtual nsresult GetControllerForCommand(const char *aCommand,
>                                             nsIController** aResult) = 0;
>    virtual nsresult GetControllers(nsIControllers** aResult) = 0;
>  
> +  virtual void GetEnabledCommands(nsTArray<nsCString>& aEnabledCommands,

Shouldn't this be called GetEnabledDisabledCommnads?

::: dom/base/nsWindowRoot.cpp
@@ +281,5 @@
>    return NS_OK;
>  }
>  
> +void
> +nsWindowRoot::GetEnabledDisabledCommands(nsIControllers* aControllers,

And then perhaps we can call this GetEnabledDisabledCommandsForController?

@@ +296,5 @@
> +    nsCOMPtr<nsICommandController> commandController(do_QueryInterface(controller));
> +    if (commandController) {
> +      uint32_t commandCount;
> +      char** commands;
> +      if (NS_SUCCEEDED(commandController->GetSupportedCommands(&commandCount, &commands))) {

You should free commands when you're done, I believe.

@@ +304,5 @@
> +          // priority.
> +          if (!aCommandsHandled.Contains(commands[e])) {
> +            aCommandsHandled.PutEntry(commands[e]);
> +
> +            bool enabled;

Please init to false to protect against IsCommandEnabled failing.

::: dom/interfaces/base/moz.build
@@ +31,5 @@
>      'nsIFocusManager.idl',
>      'nsIFrameRequestCallback.idl',
>      'nsIIdleObserver.idl',
>      'nsIQueryContentEventResult.idl',
> +    'nsIRemoteBrowser.idl',

This file seems to be missing from the patch.

::: embedding/components/commandhandler/nsControllerCommandTable.cpp
@@ +192,5 @@
> +AddCommand(const nsACString& aKey, nsIControllerCommand* aData, void* aArg)
> +{
> +  char*** commands = static_cast<char ***>(aArg);
> +  (**commands) = ToNewCString(aKey);
> +  (*commands)++;

This could use a short comment.

@@ +200,5 @@
> +NS_IMETHODIMP
> +nsControllerCommandTable::GetSupportedCommands(uint32_t* aCount,
> +                                               char*** aCommands)
> +{
> +  char** commands = new char *[mCommandsTable.Count()];

Isn't this bending the rules?  I'm pretty sure you're supposed to allocate these arrays using NS_Alloc.  (And they should be deallocated using NS_FREE_XPCOM_ALLOCATED_POINTER_ARRAY)

::: toolkit/modules/RemoteController.jsm
@@ +1,1 @@
> +  // -*- indent-tabs-mode: nil; js-indent-level: 2 -*-

This looks unintentional.

@@ +36,5 @@
>    },
>  
> +  onEvent: function () {},
> +
> +  enableDisableCommands: function(aAction,

Please add a comment clarifying that this is intended to be called from the remote-browser XBL binding.  This confused me a bit in the beginning.

@@ +49,5 @@
> +    }
> +
> +    for (let c = 0; c < aDisabledLength; c++) {
> +      this._supportedCommands[aDisabledCommands[c]] = false;
> +    }

Do we need to somehow protect against the same command appearing in both lists?

Comment 76

[Neil Deakin]

Attachment: Update commands, changing nsIController interface, v1.3

Ehsan is away so asking Olli to finish the review here.

Comment 77

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8515550 [details] [diff] [review]
Update commands, changing nsIController interface, v1.3

>+TabParent::RecvEnableDisableCommands(const nsString& aAction,
>+                                     const nsTArray<nsCString>& aEnabledCommands,
>+                                     const nsTArray<nsCString>& aDisabledCommands)
>+{
>+  nsCOMPtr<nsIRemoteBrowser> remoteBrowser = do_QueryInterface(mFrameElement);
>+  if (remoteBrowser) {
>+    nsAutoArrayPtr<const char *> enabledCommands, disabledCommands;
Nit, const char*

>+static PLDHashOperator
>+AddCommand(const nsACString& aKey, nsIControllerCommand* aData, void* aArg)
>+{
>+  // aArg is a pointer to a array of strings. It gets incremented after
>+  // allocating each one so that it points to the next location for AddCommand
>+  // to assign a string to.
>+  char*** commands = static_cast<char ***>(aArg);
Nit, static_cast<char ***>


>+  // This is intended to be called from the remote-browser binding to update
>+  // the enabled and disabled commands.
>+  enableDisableCommands: function(aAction,
>+                                  aEnabledLength, aEnabledCommands,
>+                                  aDisabledLength, aDisabledCommands)
>+  {
Shouldn't { be in the previous line in js code?

Comment 78

[Neil Deakin]

Attachment: Update commands, changing nsIController interface, v1.4

This addresses the comments above and adds some very minor fixes for compiler-specific errors that don't occur for me locally.

The remaining issue is that random but frequent NS_NOINTERFACE errors pop up when running certain e10s-browser-chrome tests, as shown here https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=90a4fe089c18

I can reproduce reliably with the test browser_install.js locally. The problem occurs when trying to QI to nsIRemoteBrowser on the remote-browser element. This calls into nsBindingManager::GetBindingImplementation to get the interfaces implemented by the xbl binding. This calls into itself recursively and throws NS_NOINTERFACE. (via AggregatedQueryInterface, then into CallQueryInterfaceOnJSObject which reports this as an exception).

Not sure what is happening here or how to fix this.

Comment 79

[Bobby Holley (:bholley)]

(In reply to Neil Deakin from comment #78)
> I can reproduce reliably with the test browser_install.js locally. The
> problem occurs when trying to QI to nsIRemoteBrowser

Hm, did you mean something else? I've never heard of such an interface. and MXR gives zero hits.

Here are the JS-implemented interfaces that remote-browser and browser implement:

http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/remote-browser.xml#13
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/browser.xml#20

 on the remote-browser
> element. This calls into nsBindingManager::GetBindingImplementation to get
> the interfaces implemented by the xbl binding. This calls into itself
> recursively and throws NS_NOINTERFACE.

Are you talking about this code? http://mxr.mozilla.org/mozilla-central/source/dom/xbl/nsBindingManager.cpp#599

> (via AggregatedQueryInterface, then
> into CallQueryInterfaceOnJSObject which reports this as an exception).

Is the error thrown in JS or C++? If JS, where? If C++, can you post a callstack of where it happens?

Comment 80

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Bobby Holley (:bholley) from comment #79)
> Hm, did you mean something else? I've never heard of such an interface. and
> MXR gives zero hits.
It is added in the patch.

Comment 81

[Neil Deakin]

(In reply to Bobby Holley (:bholley) from comment #79)
> (In reply to Neil Deakin from comment #78)
> > I can reproduce reliably with the test browser_install.js locally. The
> > problem occurs when trying to QI to nsIRemoteBrowser
> 
> Hm, did you mean something else? I've never heard of such an interface. and
> MXR gives zero hits.

It is added by the patch in this bug.
> 
> Are you talking about this code?
> http://mxr.mozilla.org/mozilla-central/source/dom/xbl/nsBindingManager.
> cpp#599

Yes, the block of code immediately after returns NS_NOINTERFACE.

I doesn't occur in debug builds, and I can't reproduce it in a debugger. This is the stack from http://mxr.mozilla.org/mozilla-central/source/dom/xbl/nsBindingManager.cpp#621


#01: _ZN16nsBindingManager24GetBindingImplementationEP10nsIContentRK4nsIDPPv[/builds/moz2/working/output-opt/dist/Nightly.app/Contents/MacOS/XUL +0x15adea0]
#02: _ZN12nsXULElement14QueryInterfaceERK4nsIDPPv[/builds/moz2/working/output-opt/dist/Nightly.app/Contents/MacOS/XUL +0x1629318]
#03: _ZN7mozilla3dom14QueryInterfaceEP9JSContextjPN2JS5ValueE[/builds/moz2/working/output-opt/dist/Nightly.app/Contents/MacOS/XUL +0x1118be9]
#04: _ZN2js6InvokeEP9JSContextN2JS8CallArgsENS_14MaybeConstructE[/builds/moz2/working/output-opt/dist/Nightly.app/Contents/MacOS/XUL +0x26b6bc4]
#05: _ZN2js6InvokeEP9JSContextRKN2JS5ValueES5_jPS4_NS2_13MutableHandleIS3_EE[/builds/moz2/working/output-opt/dist/Nightly.app/Contents/MacOS/XUL +0x26a565f]
#06: _Z20JS_CallFunctionValueP9JSContextN2JS6HandleIP8JSObjectEENS2_INS1_5ValueEEERKNS1_16HandleValueArrayENS1_13MutableHandleIS6_EE[/builds/moz2/working/output-opt/dist/Nightly.app/Contents/MacOS/XUL +0x25bbc0e]
#07: _ZN19nsXPCWrappedJSClass28CallQueryInterfaceOnJSObjectEP9JSContextP8JSObjectRK4nsID[/builds/moz2/working/output-opt/dist/Nightly.app/Contents/MacOS/XUL +0x58ce95]
#08: _ZN19nsXPCWrappedJSClass23DelegatedQueryInterfaceEP14nsXPCWrappedJSRK4nsIDPPv[/builds/moz2/working/output-opt/dist/Nightly.app/Contents/MacOS/XUL +0x58d532]
#09: _ZN16nsBindingManager24GetBindingImplementationEP10nsIContentRK4nsIDPPv[/builds/moz2/working/output-opt/dist/Nightly.app/Contents/MacOS/XUL +0x15adbc3]
#10: _ZN12nsXULElement14QueryInterfaceERK4nsIDPPv[/builds/moz2/working/output-opt/dist/Nightly.app/Contents/MacOS/XUL +0x1629318]
#11: _ZN13nsCOMPtr_base14assign_from_qiE16nsQueryInterfaceRK4nsID[/builds/moz2/working/output-opt/dist/Nightly.app/Contents/MacOS/XUL +0x9ec20]
#12: _ZN7mozilla3dom9TabParent25RecvEnableDisableCommandsERK8nsStringRK8nsTArrayI9nsCStringES9_[/builds/moz2/working/output-opt/dist/Nightly.app/Contents/MacOS/XUL +0x152f86e]
#13: _ZN7mozilla3dom14PBrowserParent17OnMessageReceivedERKN3IPC7MessageE[/builds/moz2/working/output-opt/dist/Nightly.app/Contents/MacOS/XUL +0x36c2b0]
#14: _ZN7mozilla3dom14PContentParent17OnMessageReceivedERKN3IPC7MessageE[/builds/moz2/working/output-opt/dist/Nightly.app/Contents/MacOS/XUL +0x3c79ee]

> 
> > (via AggregatedQueryInterface, then
> > into CallQueryInterfaceOnJSObject which reports this as an exception).
> 
> Is the error thrown in JS or C++? If JS, where? If C++, can you post a
> callstack of where it happens?

Comment 82

[Bobby Holley (:bholley)]

Finally managed to reproduce this locally. Looking at it now.

Comment 83

[Bobby Holley (:bholley)]

Fix is in bug 1095587.

The basic problem is that XPCWrappedJS QI is supposed to detect NS_NOINTERFACE and squash it. It does this (in nsXPCWrappedJSClass::CallQueryInterfaceOnJSObject) by doing:

UNWRAP_OBJECT(Exception, &jsexception.toObject(), e);

However, we were actually getting an XPCWrappedNative_NoHelper (rather than a DOM object), causing this to fail. This was clearly wrong, and turned out to be because Exception doesn't QI to nsWrapperCache, even though it should.

The intermittent and opt-only-ness is probably due to the fact that we may or may not hit the recursion bailout in GetBindingImplementation depending on the order and timing with which bindings and interfaces are set up.

Comment 84

[Neil Deakin]

Attachment: Update commands, changing nsIController interface, v1.5

Final version. Minor change to use GetDocShell instead of mDocShell in UpdateCommands. Also disabled the test on windows due to intermittent failures. Will look at it when I get back from vacation.

Comment 85

[Neil Deakin]

https://hg.mozilla.org/integration/mozilla-inbound/rev/41536afea9b3
https://hg.mozilla.org/integration/mozilla-inbound/rev/a84fe0cefd77

Comment 86

[Dave Townsend [:mossop]]

(In reply to Neil Deakin from comment #85)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/41536afea9b3
> https://hg.mozilla.org/integration/mozilla-inbound/rev/a84fe0cefd77

WOOT!

Comment 87

[Carsten Book [:Tomcat]]

sorry had to back this out, since we had frequent test failures like https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3719174&repo=mozilla-inbound starting with the landing of this bug

Comment 88

[[e-mail]]

(In reply to Neil Deakin from comment #64)
> I don't agree, but I'm hoping Neil can review this before I make any more
> changes.

Sorry, for some reason I thought I'd already previously reviewed something here, and I couldn't work out what was new, and smaug didn't seem to like your approach anyway.

Comment 89

[Neil Deakin]

Attachment: Update commands, changing nsIController interface, improve test, v1.6

Comment 90

[Neil Deakin]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d4f962fb38be
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ddd9eb25925

Comment 91

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This apparently created a new intermittent bc1 orange, so backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/7ed611fb2797 before it gets out of control

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=4435851&repo=mozilla-inbound

Comment 92

[Neil Deakin]

I checked this in again without the test.

https://hg.mozilla.org/integration/mozilla-inbound/rev/9007304a46e3
https://hg.mozilla.org/integration/mozilla-inbound/rev/6940b3bf61ad

Comment 93

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9007304a46e3
https://hg.mozilla.org/mozilla-central/rev/6940b3bf61ad

Comment 94

[Vasilica Tamas, QA [:vtamas]]

I was able to reproduce this issue on Firefox 34.0a1 (2014-08-29) using Mac OSX 10.9.5.

Verified fixed on Latest Firefox 37.0a1 (2014-12-15) using Mac OSX 10.9.5, Ubuntu 14.04 x86 and on Latest Firefox 37.0a1 (2014-12-14) using Windows 7 x64.

Comment 96

[Firefox Bug Husbandry Bot]

Moving to Core:XUL per https://bugzilla.mozilla.org/show_bug.cgi?id=1455336