[e10s] BrowserUtils.jsm spams the web console with "unsafe CPOW usage" warnings because of getFocusSync

RESOLVED FIXED in Firefox 40

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: froydnj, Assigned: enndeakin)

Tracking

(Blocks 1 bug)

Trunk
mozilla40
Points:
3
Dependency tree / graph

Firefox Tracking Flags

(e10sm8+, firefox40 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Reporter

Description

4 years ago
I see a lot of messages about BrowserUtils.jsm causing unsafe CPOW usage; the offending line is:

http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/BrowserUtils.jsm#100

which, on the one hand, is great that the function getFocusSync has comments explicitly stating that it returns a CPOW.  But it's not so great that it spams the console.
Assignee

Comment 1

4 years ago
Posted patch Patch, needs some testing (obsolete) — Splinter Review
Assignee: nobody → enndeakin
Status: NEW → ASSIGNED
Assignee

Updated

4 years ago
Iteration: --- → 40.3 - 11 May
Points: --- → 3
Assignee

Comment 2

4 years ago
Attachment #8597244 - Attachment is obsolete: true
Attachment #8598593 - Flags: review?(mconley)
Comment on attachment 8598593 [details] [diff] [review]
Rework selection retrieval to not use wrappers and remove getFocusSync

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

This looks good - but withholding r+ until I hear Neil's opinion on deprecating getBrowserSelection.

::: browser/base/content/browser.js
@@ -5284,5 @@
> - *
> - * @param aCharLen
> - *        The maximum number of characters to return.
> - */
> -function getBrowserSelection(aCharLen) {

I've found evidence of at least a few add-ons using this function. I haven't gone so far as to determine how popular those add-ons are, but I wonder if we might leave this in here with a deprecation warning (a la Deprecated.jsm).

I think it'd be OK if it didn't work with e10s - if it threw or something if the focused element is a remote browser.

Does that sound OK to you?

::: browser/base/content/content.js
@@ +157,5 @@
>        referrer: referrer,
>        referrerPolicy: referrerPolicy,
>        contentType: contentType,
>        contentDisposition: contentDisposition,
> +      selectionInfo: selectionInfo

Nit - please add a trailing comma.

::: browser/base/content/nsContextMenu.js
@@ +544,5 @@
> +    }
> +    else {
> +      this.selectionInfo = BrowserUtils.getSelectionDetails(window);
> +    }
> +    

Nit - trailing whitespace

::: toolkit/modules/BrowserUtils.jsm
@@ +302,5 @@
> +    let focusedElement = Services.focus.getFocusedElementForWindow(topWindow, true, focusedWindow);
> +    focusedWindow = focusedWindow.value;
> +
> +    let selection = focusedWindow.getSelection();
> +    let selectionStr = focusedWindow.getSelection().toString();

Might as well use selection.toString()
Assignee

Comment 4

4 years ago
Sounds ok
Attachment #8598593 - Attachment is obsolete: true
Attachment #8598593 - Flags: review?(mconley)
Attachment #8598715 - Flags: review?(mconley)
Comment on attachment 8598715 [details] [diff] [review]
Rework selection retrieval to not use wrappers and remove getFocusSync, v2

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

Thanks Neil!

::: browser/base/content/browser.js
@@ +5281,3 @@
>  function getBrowserSelection(aCharLen) {
> +  Deprecated.warning("getBrowserSelection",
> +                     "https://bugzilla.mozilla.org/show_bug.cgi?id=1134769");

We should file a follow-up bug to remove the function entirely.

And we should probably link to documentation on MDN with an alternative solution (probably using messages).

Maybe just stub something out on MDN and dev-doc-needed it?
Attachment #8598715 - Flags: review?(mconley) → review+
https://hg.mozilla.org/mozilla-central/rev/032a7f964193
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.