Get rid of the contextmenu sync IPC

ASSIGNED
Assigned to

Status

()

Firefox
Menus
ASSIGNED
4 months ago
2 days ago

People

(Reporter: Ehsan, Assigned: Perry)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qf:p1][fce-active])

Attachments

(1 attachment)

I think we should consider killing the contextmenu sync IPC.  Telemetry data shows that the median time on Nightly is 37ms, and the submission percentage is 68%.  This seems like a pretty common occurrence.
Mike, I remember we briefly talked about this in the QF work week.  Is this doable?
Flags: needinfo?(mconley)
I traced the sync message all the way back to bug 1030451 where it was first introduced for spell checking, but I'm not sure why. billm, do you remember why a sync message was required here?
Flags: needinfo?(mconley) → needinfo?(wmccloskey)
It has to be sync (really rpc) or else all the CPOWs we send down from nsContextMenu.js would be unsafe.

Before Ally's patch, we used unsafe CPOWs. That sort of worked. However, for spell checking, I think rangeParent and rangeOffset (or maybe just one of them) are no longer valid in the child after the context menu event has fired. So we would try to access them and get null or something.

We'll just need to package whatever information we get from rangeParent/rangeOffset into the "contextmenu" message we send from the content script.
Flags: needinfo?(wmccloskey)
(In reply to Bill McCloskey (:billm) from comment #3)
> It has to be sync (really rpc) or else all the CPOWs we send down from
> nsContextMenu.js would be unsafe.
> 
> Before Ally's patch, we used unsafe CPOWs. That sort of worked. However, for
> spell checking, I think rangeParent and rangeOffset (or maybe just one of
> them) are no longer valid in the child after the context menu event has
> fired. So we would try to access them and get null or something.

Why are they not valid?  As far as I can tell they get passed to here: <https://searchfox.org/mozilla-central/rev/6580dd9a4eb0224773897388dba2ddf5ed7f4216/toolkit/modules/InlineSpellCheckerContent.jsm#38> which means we hold on to them here <https://searchfox.org/mozilla-central/rev/6580dd9a4eb0224773897388dba2ddf5ed7f4216/toolkit/modules/InlineSpellChecker.jsm#85>.
Oh, I guess all this code changed in bug 1026099 and we don't use CPOWs for spell checking anymore.

Anyway, we still need this message to be Rpc as long as we use any CPOWs in the context menu code.
Whiteboard: [qf]

Updated

2 months ago
Flags: needinfo?(mconley)
So I was asked to look at this to see whether the sync message can be removed.

Yes, I think so. nsContextMenu.js will, however, need to be refactored to stop relying on these two CPOWs:

http://searchfox.org/mozilla-central/rev/1b7b1ec949497e9fc2a9b9dfaccbe894ee2ad5ef/browser/base/content/nsContextMenu.js#39-40

I actually don't see popupNode being used at all except for here in this test:

http://searchfox.org/mozilla-central/rev/1b7b1ec949497e9fc2a9b9dfaccbe894ee2ad5ef/browser/base/content/test/general/browser_addKeywordSearch.js#30-32

So it can probably be removed and the test modified to not rely on it.

The gContextMenuContentData.event CPOW is, however, relied upon during the creation of the nsContextMenu for the following things:

1) The mozInputSource (which can easily be serialized and sent up on its own instead)
2) The target of the event (the thing that was clicked... this appears to be used all over the place for various reasons)
3) The range parent and range offset of any selections (this appears to be used for spellcheck at least)

Once that's done, we'll probably also need to add some code to deal with the possibility of a content process sending up multiple context menu messages (since it'll no longer be blocked).

So I don't think there's anything preventing us from getting rid of these CPOWs and this sync message except a lot of mucking around in nsContextMenu.js to not rely on DOM node CPOWs. If we can find somebody with the cycles to do that, this should be reasonably straight-forward.
Flags: needinfo?(mconley)
Whiteboard: [qf] → [qf:p1]
Hey felipe, do you think this would be something Perry would find interesting taking?

Updated

a month ago
Flags: needinfo?(felipc)
Yep, he is interested!
Assignee: nobody → jiangperry
Status: NEW → ASSIGNED
Flags: needinfo?(felipc)

Updated

a month ago
Whiteboard: [qf:p1] → [qf:p1][fce-active]
(Assignee)

Comment 9

28 days ago
Created attachment 8888965 [details] [diff] [review]
Bug 1360406 - Create context menu module and remove context menu target CPOW

CastingApps methods still use CPOW
Attachment #8888965 - Flags: review?(felipc)
Comment on attachment 8888965 [details] [diff] [review]
Bug 1360406 - Create context menu module and remove context menu target CPOW

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

So I did a big architectural review here to understand how things are structured, and also looked a bit into the specifics. I'm pretty happy with the way this shaped, and the main things I'd like to see a bit different to move forward are:

- In the content side, the ContextMenu object lives for the lifetime of the tab. Could we have it created at the moment that a menu will be created, and then destroy that object reference (by setting it to null) when the we get the PopupHiding notification? I remember we talked about this and there was some problem about getting this message a little bit early and then needing some extra reference, but we can perhaps find a way to delay the object destruction until no more info will be needed.

- Find a way to unify the sendAsyncMessage for the non-remote case (commented in more details below)

- I like your use of JS object prototypes, and while I like that style myself, we're moving away from that in the codebase in favor of using ES6 Classes. It's already supported in our JS engine and there's code in the tree that uses that already. How much work do you think would be to change ContextMenu.jsm to use ES6 Classes? If we get that it'd be pretty sweet! And it would be a good exercise for you to use this new feature.

- Of course, we'll need to remove all the commented out code before landing. It's probably a good point now to remove that, and run a `mach eslint` to make sure that all the new code conforms to the enforced styling rules, otherwise it will bounce when it lands.

::: browser/base/content/content.js
@@ +734,5 @@
>  
>  ContentLinkHandler.init(this);
>  
>  // TODO: Load this lazily so the JSM is run only if a relevant event/message fires.
> +var pluginContent = new PluginContent(global, contextMenu);

with the current code in PluginContent.jsm you won't need to pass this contextMenu param anymore

::: browser/base/content/nsContextMenu.js
@@ +156,5 @@
>    },
>  
> +  setContext() {
> +    if (gContextMenuContentData) {
> +      Object.assign(this, gContextMenuContentData.context);

I see what this is doing, and it's pretty nice, but it makes it harder when searching/reading the code to see where this properties are coming from.

As painful as it is, I think it would be better to set the properties one by one so that it's explicit in the code, and also findable when searching for, e.g., "this.onImage" on searchfox.

::: browser/modules/ContextMenu.jsm
@@ +624,5 @@
> +      popupNodeSelectors,
> +      disableSetDesktopBg,
> +      parentAllowsMixedContent,
> +    });
> +  } else {

Even in the non-remote case, it's still possible to send messages from the "content" side (from things coming from the content.js script) to the "parent" side (the same code that is receiving this in the e10s case, i.e., tabbrowser.xml). So the sendAsyncMessage here, and the mainWin.gContextMenuContentData part below, could be merged. That's the preferred pattern for this type of things when we have to handle both remote and non-remote cases.

::: browser/modules/PluginContent.jsm
@@ +35,5 @@
>      this.global = global;
>      // Need to hold onto the content window or else it'll get destroyed
>      this.content = this.global.content;
> +    // Used to access the context menu target
> +    this.contextMenu = this.global.contextMenu;

can we avoid storing this reference here, and directly access this.global.contextMenu when needed?

::: toolkit/content/contentAreaUtils.js
@@ +449,5 @@
>      let isPrivate = aIsContentWindowPrivate;
>      if (isPrivate === undefined) {
>        isPrivate = aInitiatingDocument instanceof Components.interfaces.nsIDOMDocument
> +                    ? PrivateBrowsingUtils.isContentWindowPrivate(aInitiatingDocument.defaultView)
> +                    : aInitiatingDocument.isPrivate;

nit: as this is just a formatting change, so let's drop this from this patch
Attachment #8888965 - Flags: review?(felipc) → feedback+
You need to log in before you can comment on or make changes to this bug.