Closed Bug 1199239 Opened 4 years ago Closed 4 years ago

[e10s] Clicking "bookmark this page" button (star) causes unsafe CPOW usage warning

Categories

(Firefox :: Bookmarks & History, defect, P1)

defect
Points:
3

Tracking

()

VERIFIED FIXED
Firefox 44
Tracking Status
e10s m8+ ---
firefox43 --- affected
firefox44 --- verified

People

(Reporter: arni2033, Assigned: enndeakin)

References

(Blocks 1 open bug)

Details

(Whiteboard: [bugday-20151021])

Attachments

(2 files)

STR:   (Win7_64bit, Nightly 43.0a1 32bit   ID 20150826030211, new profile)

1. Open http://example.org/ in latest Nightly with multiprocess mode ON
2. Click "bookmark this page" button (star)

Result:   This causes some "unsafe CPOW usage" warnings in the Browser Console.
tracking-e10s: --- → ?
Assignee: nobody → enndeakin
Status: NEW → ASSIGNED
Points: --- → 3
Also fixes some tests that relied on this happening synchronously.
Attachment #8662136 - Flags: review?(mak77)
Comment on attachment 8662136 [details] [diff] [review]
Get page info from content process

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

I'll take a second look, the approach is good

::: browser/base/content/browser-places.js
@@ +406,5 @@
>        // Bug 1148838 - Make this code work for full page plugins.
>        let description = null;
>        let charset = null;
> +
> +      let docInfo = yield this._getPageDescription();

should it take aBrowser as argument? it seems to always use gBrowser internally, that doesn't sound right.

@@ +469,5 @@
>      }
>    }),
>  
> +  _getPageDescription()
> +  {

I know we use different styles all around, but since it was decided the proper style is to brace on the same line (unless it's CPP class methods), please move up the brace.

this is returning far more information than the description (that is a generic word, but usually means the "meta" description), I think we want something more generic... I'd use something like_getPageInfo or _getPageDetails. The message should clearly be renamed after it.

@@ +471,5 @@
>  
> +  _getPageDescription()
> +  {
> +    return new Promise(resolve => {
> +      let mm = gBrowser.selectedBrowser.messageManager;

use argument browser, as said

@@ +473,5 @@
> +  {
> +    return new Promise(resolve => {
> +      let mm = gBrowser.selectedBrowser.messageManager;
> +      mm.addMessageListener("Bookmarks:GetPageDescription:Result", function listenForFocus(msg) {
> +        mm.removeMessageListener("Bookmarks:GetPageDescription:Result", listenForFocus);

I think "listenForFocus" is a copy/past miss, a generic listener() will do.

@@ +585,5 @@
>     */
>    addLiveBookmark: function PCH_addLiveBookmark(url, feedTitle, feedSubtitle) {
>      var feedURI = makeURI(url);
>  
> +    var title = (arguments.length > 1) ? feedTitle : aBrowser.contentTitle;

while here please replace "var" with "let" here, for consistency with new code.
Note that aBrowser here doesn't exist, should be gBrowser.

since this actually makes this method async, we should convert it to Task.async and

let title = feedTitle || gBrowser.contentTitle;
let description = feedSubtitle;
if (!description) {
  description = (yield this._getPageDetails(gBrowser)).description;
}
PlacesUIUtils.showBookmarksDialog...

after doing that, please add a .catch(Components.utils.reportError) at http://mxr.mozilla.org/mozilla-central/source/browser/components/feeds/FeedConverter.js#427

@@ +590,3 @@
>  
> +    function showDialog(docInfo) {
> +      var toolbarIP = new InsertionPoint(PlacesUtils.toolbarFolderId,

note this should happen before the yield, we want the insertion point when the original method is called.

::: browser/base/content/test/general/browser_bookmark_titles.js
@@ +83,5 @@
>  }
> +
> +// BrowserTestUtils.browserLoaded doesn't work for the about pages, so use a
> +// custom page load listener.
> +function promisePageLoaded(browser)

nit: could probably use promiseTabLoadEvent, if you have time to do that, otherwise nvm.

::: browser/base/content/test/general/head.js
@@ +1080,5 @@
> +/**
> + * Resolves when a bookmark with the given uri is added.
> + */
> +function promiseOnBookmarkItemAdded(aExpectedURI) {
> +  let defer = Promise.defer();

Promise.defer() is deprecated, please convert this to return new Promise()
Attachment #8662136 - Flags: review?(mak77) → feedback+
Attachment #8665410 - Flags: review?(mak77)
> nit: could probably use promiseTabLoadEvent, if you have time to do that, otherwise nvm.

This didn't seem to work either.
Comment on attachment 8665410 [details] [diff] [review]
Get page info from content process, v2

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

::: browser/base/content/browser-places.js
@@ +334,5 @@
>        var title;
>        var description;
>        var charset;
> +
> +      let docInfo = yield this._getPageDetails(gBrowser.selectedBrowser);

should be aBrowser here

::: browser/components/feeds/FeedConverter.js
@@ +427,5 @@
> +      try {
> +        topWindow.PlacesCommandHook.addLiveBookmark(spec, title, subtitle);
> +      } catch (e) {
> +        Components.utils.reportError(e);
> +      }

This won't do, since addLiveBookmark returns a promise now, you need to 
topWindow.PlacesCommandHook.addLiveBookmark(spec, title, subtitle)
                           .catch(Components.utils.reportError);
Attachment #8665410 - Flags: review?(mak77) → review+
Priority: -- → P1
https://hg.mozilla.org/mozilla-central/rev/55c331a6f9da
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
I have reproduced this bug with Firefox Nightly 43.0a1 (Build ID: 20150827030213) on 
windows 8.1 64-bit with the instructions from comment 0 .

Verified as fixed with Firefox Nightly 44.0a1(Build ID: 20151020031317)

Mozilla/5.0 (Windows NT 6.3; WOW64; rv:44.0) Gecko/20100101 Firefox/44.0
Whiteboard: [bugday-20151021]
Reproduced this bug on Nightly  43.0a1 (2015-08-27) in Linux,64 bit

This Bug is now verified as fixed on Latest Firefox Nightly 44.0a1 (2015-10-24)

Build ID: 20151024030248
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:44.0) Gecko/20100101 Firefox/44.0

As it is also verified on Windows (Comment 8), Marking it as verified!
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.