Closed
Bug 1360406
Opened 7 years ago
Closed 7 years ago
Get rid of the contextmenu sync IPC
Categories
(Firefox :: Menus, enhancement)
Firefox
Menus
Tracking
()
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.
Reporter | ||
Comment 1•7 years ago
|
||
Mike, I remember we briefly talked about this in the QF work week. Is this doable?
Flags: needinfo?(mconley)
Comment 2•7 years ago
|
||
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)
Reporter | ||
Comment 4•7 years ago
|
||
(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.
Reporter | ||
Updated•7 years ago
|
Whiteboard: [qf]
Updated•7 years ago
|
Flags: needinfo?(mconley)
Comment 6•7 years ago
|
||
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)
Updated•7 years ago
|
Whiteboard: [qf] → [qf:p1]
Comment 7•7 years ago
|
||
Hey felipe, do you think this would be something Perry would find interesting taking?
Updated•7 years ago
|
Flags: needinfo?(felipc)
Comment 8•7 years ago
|
||
Yep, he is interested!
Assignee: nobody → jiangperry
Status: NEW → ASSIGNED
Flags: needinfo?(felipc)
Updated•7 years ago
|
Whiteboard: [qf:p1] → [qf:p1][fce-active]
Assignee | ||
Comment 9•7 years ago
|
||
CastingApps methods still use CPOW
Attachment #8888965 -
Flags: review?(felipc)
Comment 10•7 years ago
|
||
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 hidden (mozreview-request) |
Comment 12•7 years ago
|
||
mozreview-review |
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+
Comment 13•7 years ago
|
||
\o/ Great stuff here, Perry!
Reporter | ||
Comment 14•7 years ago
|
||
Hi Perry, what's the status of this bug? Is this ready to land? Thanks!
Comment 15•7 years ago
|
||
Perry's internship has ended, so fwding to Felipe. :\
Flags: needinfo?(felipc)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 17•7 years ago
|
||
(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).
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8888965 -
Attachment is obsolete: true
Comment 19•7 years ago
|
||
Thanks, Perry!
Comment 20•7 years ago
|
||
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
Updated•7 years ago
|
Flags: needinfo?(felipc)
Comment 21•7 years ago
|
||
Pushed by felipc@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/70dca88c5cc3 Remove context menu sync IPC. r=felipe
Comment 22•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/70dca88c5cc3
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Updated•6 years ago
|
Whiteboard: [qf:p1][fce-active] → [qf:p1][fce-active-legacy]
Updated•2 years ago
|
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.
Description
•