Closed Bug 1360406 Opened 7 years ago Closed 7 years ago

Get rid of the contextmenu sync IPC

Categories

(Firefox :: Menus, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 57
Performance Impact high
Tracking Status
firefox57 --- fixed

People

(Reporter: ehsan.akhgari, Assigned: perry)

References

(Blocks 1 open bug)

Details

(Whiteboard: [fce-active-legacy])

Attachments

(1 file, 1 obsolete file)

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]
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?
Flags: needinfo?(felipc)
Yep, he is interested!
Assignee: nobody → jiangperry
Status: NEW → ASSIGNED
Flags: needinfo?(felipc)
Whiteboard: [qf:p1] → [qf:p1][fce-active]
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+
Comment on attachment 8899894 [details]
Bug 1360406 - Remove context menu sync IPC

https://reviewboard.mozilla.org/r/171222/#review178066

Awesome! Really great work! And thanks for converting the module to ES6 Classes too.

To land let's remove all the TODO's related to the CastingApps through bug 1393582 (it can be a patch on top of this one), and then we can land both together
Attachment #8899894 - Flags: review?(felipc) → review+
\o/ Great stuff here, Perry!
Hi Perry, what's the status of this bug?  Is this ready to land?  Thanks!
Perry's internship has ended, so fwding to Felipe. :\
Flags: needinfo?(felipc)
(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog, Away until Sep 12) from comment #14)
> Hi Perry, what's the status of this bug?  Is this ready to land?  Thanks!

I believe it is, but I'll let Felipe double check since some time has passed. I checked that a rebase works and just submitted a new request with CastingApps usage removed (per bug 1393582).
Attachment #8888965 - Attachment is obsolete: true
Thanks, Perry!
Thanks a lot for the rebase, Perry! I just pushed this one last time to try (together with bug 1393582), and assuming it works, I'll land it!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=98aaa27bc59d6116c1fbc10e88ae4adaf5b4e49b
Flags: needinfo?(felipc)
Pushed by felipc@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/70dca88c5cc3
Remove context menu sync IPC. r=felipe
https://hg.mozilla.org/mozilla-central/rev/70dca88c5cc3
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Depends on: 1403127
Depends on: 1399429
Whiteboard: [qf:p1][fce-active] → [qf:p1][fce-active-legacy]
Performance Impact: --- → P1
Whiteboard: [qf:p1][fce-active-legacy] → [fce-active-legacy]
You need to log in before you can comment on or make changes to this bug.