1135619 - [e10s] "Bookmark this Frame" in remote browser fails, causes NS_ERROR_XPC_CANT_PASS_CPOW_TO_NATIVE error and unsafe CPOW usage warnings

Status: RESOLVED FIXED

(Firefox :: Menus, defect)

Description

[Ian Moody [:Kwan] (UTC+1)]

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

STR:

1) Visit a site with a frame on it in an e10s window
2) Right-click in the frame, and choose "This Frame" > "Bookmark This Frame"

This causes some "unsafe CPOW usage" warnings in the Browser Console, and an "NS_ERROR_XPC_CANT_PASS_CPOW_TO_NATIVE: It's illegal to pass a CPOW to native code arg 0 [nsINavBookmarksService.getBookmarkIdsForURI]" error, which means it fails.

In browser/base/content/nsContextMenu.js:

  addBookmarkForFrame: function CM_addBookmarkForFrame() {
    var doc = this.target.ownerDocument; <-- Causes CPOW warning
    var uri = doc.documentURIObject; <-- Causes CPOW warning
    urlSecurityCheck(this.linkURL, this.principal);


in toolkit/components/places/PlacesUtils.jsm

  getMostRecentBookmarkForURI:
  function PU_getMostRecentBookmarkForURI(aURI) {
    var bmkIds = this.bookmarks.getBookmarkIdsForURI(aURI); <-- Causes NS_ERROR_XPC_CANT_PASS_CPOW_TO_NATIVE error
    for (var i = 0; i < bmkIds.length; i++) {

Comment 1

[Ian Moody [:Kwan] (UTC+1)]

Attachment: Make "Bookmark This Frame" use messages so it doesn't cause an NS_ERROR_XPC_CANT_PASS_CPOW_TO_NATIVE

Relies on the work in bug 1133577

Doesn't currently fix the first CPOW from "this.target.ownerDocument", since I'm not sure of the best way to do so.  Pass the ownerDocument up in gContextMenuContentData? Will that not be too heavyweight?  This is also an issue for bug 1134424 (Save Image/Audio/Video).  Both places need the actual document to pass on to other code elsewhere.

Exposes a bunch more "unsafe CPOW usage" warnings in PlacesUIUtils.getDescriptionFromDocument(), which I feel should be left for another bug, or at least done later once this is working again.

also fixes a CPOW that wasn't hit before:

in browser/base/content/nsContextMenu.js

  addBookmarkForFrame: function CM_addBookmarkForFrame() {
    var doc = this.target.ownerDocument;
    var uri = doc.documentURIObject;

    var itemId = PlacesUtils.getMostRecentBookmarkForURI(uri);
    if (itemId == -1) {
      var title = doc.title; <- Causes CPOW warning
      var description = PlacesUIUtils.getDescriptionFromDocument(doc);

Comment 2

[Ian Moody [:Kwan] (UTC+1)]

Attachment: Make "Bookmark This Frame" use messages so it doesn't cause an NS_ERROR_XPC_CANT_PASS_CPOW_TO_NATIVE

Updated for latest bug 1133577 patch

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Hey Ian - did this fall through the cracks? Did this patch need review?

Comment 4

[Ian Moody [:Kwan] (UTC+1)]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #3)
> Hey Ian - did this fall through the cracks? Did this patch need review?

No, I was just posting a work-in-progress for safe-keeping (especially since I junked my dev VM and made a new one in the interim).

Should have a final patch soonish though, this and the related Bug 1141350 are at the top of my to-do list.

Comment 5

[Ian Moody [:Kwan] (UTC+1)]

Attachment: MozReview Request: bz://1135619/Kwan

/r/8387 - Bug 1135619 - Make "Bookmark This Frame" fetch the document title and description via message so it is safe in e10s

Pull down this commit:

hg pull -r d6970c40dff090662669c01c306e4085f81e7762 https://reviewboard-hg.mozilla.org/gecko/

Comment 6

[Ian Moody [:Kwan] (UTC+1)]

Comment on attachment 8571681 [details] [diff] [review]
Make "Bookmark This Frame" use messages so it doesn't cause an NS_ERROR_XPC_CANT_PASS_CPOW_TO_NATIVE

As an FYI, actually saving the bookmark currently isn't possible as the save button never activates due to bug 951651, but will be fixed by bug 1161882.

Comment 7

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8603052 [details]
MozReview Request: bz://1135619/Kwan

https://reviewboard.mozilla.org/r/8385/#review7145

Looks great!

Comment 8

[Ian Moody [:Kwan] (UTC+1)]

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3162e2bd3047
green with one intermittent bug 961215

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/606be3189b85

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/606be3189b85

Comment 12

[Ian Moody [:Kwan] (UTC+1)]

Attachment: MozReview Request: Bug 1135619 - Make "Bookmark This Frame" fetch the document title and description via message so it is safe in e10s