Closed Bug 290200 Opened 19 years ago Closed 18 years ago

dictionary search extension broken in latest nightlies

Categories

(Firefox :: General, defect)

Product:
Firefox
Component:
General
Version:
1.0 Branch
Platform:
x86
All
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED WORKSFORME

People

(Reporter: chase, Assigned: asa)

Details

Asa reports on behalf of the QA team that the dictionary search extension is
broken in the latest nightlies.  Filed in Firefox->General, feel free to reassign.
Nominating blocking-aviary1.0.3.
Flags: blocking-aviary1.0.3?
context menu items for dictionary search are empty. When I select some text and
bring up the context menu, I get this JS error.

Error: [Exception... "Illegal operation on WrappedNative prototype object" 
nsresult: "0x8057000c (NS_ERROR_XPC_BAD_OP_ON_WN_PROTO)"  location: "JS frame ::
chrome://dictionarysearch/content/dictionarysearchOverlay.js ::
dictionarySearchGetSelectedText :: line 181"  data: no]
Source File: chrome://dictionarysearch/content/dictionarysearchOverlay.js
Line: 181
from the dictionary search js

function dictionarySearchGetSelectedText(concationationChar){
    //log("dictionarySearchGetSelectedText()");
    
    var node = document.popupNode;
    var selection = "";
    var nodeLocalName = node.localName.toUpperCase();
    if ((nodeLocalName == "TEXTAREA") || (nodeLocalName == "INPUT" && node.type
== "text")) {
        selection = node.value.substring(node.selectionStart, node.selectionEnd);
    } 
    else {
        var focusedWindow = document.commandDispatcher.focusedWindow;
        selection =
focusedWindow.__proto__.getSelection.call(focusedWindow).toString();
    }
Someone copied a bad pattern from Firefox chrome.  In order to workaround name
collisions between a content-defined getSelection function and the DOM window
method of the same name, XUL browsers have had to use either XPCNativeWrapper
(the better way, but still error-prone and likely to be not used when most
needed) or the utter hack hyatt or blake used:

selection = focusedWindow.__proto__.getSelection.call(focusedWindow);

Thanks to patching done by jst and me in a followup to bug 217195, neither of
these kludges is needed any longer for secure access from chrome (browser or
extension) to content, without fear of calling a content-defined function
instead of the DOM standard one.  So the above can be simplified to:

selection = focusedWindow.getSelection();

Extension authors should not be calling content.__proto__.f.call for any
function f; likewise for content DOM objects other than the window named in
chrome by content or _content (its old name).

Asa, reassigning to you to turn this into an evang bug.  Please let dria know if
she can help in doc'ing this.

/be
Assignee: jst → asa
(In reply to comment #4)
> Thanks to patching done by jst and me in a followup to bug 217195, neither of
> these kludges is needed any longer for secure access from chrome (browser or
> extension) to content, without fear of calling a content-defined function
> instead of the DOM standard one.  So the above can be simplified to:
> 
> selection = focusedWindow.getSelection();

In 1.0.3 and later... But to write extensions that work safely with older
versions they'll need to use XPCNativeWrapper, or do so conditionally based on
version. Or should extension authors simply rely on the safety of Firefox and
urge users to upgrade?

Not many extensions should need revision for 1.0.3.  In the particular case at
hand, I bet no one remembers what page or pages define their own getSelection. 
So I question whether, without more data, it's even worthwhile for extension
authors to care about using XPCNativeWrapper (*not* __proto__.getSelection.call
which was always bogus) for 1.0-1.0.2.  I bet this pattern was copied from our
chrome without a pressing need for it.

XPCNativeWrapper is still good to use, esp. if it's important to avoid ad-hoc JS
properties set by content that do not shadow an XPConnected member of the same
name.  We are not going to remove XPCNativeWrapper, even though we've relieved
XUL authors of the need to use it to be secure when accessing content DOMs.

So cc'ing caillon, who can help dria write up something about XPCNativeWrapper.

/be
dria: just in case it's unclear, dveditz was making a two-fold point here:

"But to write extensions that work safely with older versions they'll need to
use XPCNativeWrapper, ...."

Before 1.0.3, calling content.getSelection or focusedWindow.getSelection was
both potentially calling the wrong method (in the unlikely but possible case
that the content author declared a getSelection global function shadowing the
DOM standard one), and unsafe.

Calling focusedWindow.__proto__.getSelection.call(...) was also unsafe, although
it would dodge a name collision in focusedWindow (users are free to declare
__proto__ too, so it wasn't foolproof even on the name-collision-avoidance
front, never mind safety).

The analogous, safe, and unambiguous in light of content name overrides
XPCNativeWrapper usage example should come from caillon, lest I botch it.

/be
> (users are free to declare __proto__ too,

Argh!  I meant "decorate" (with ad-hoc properties shadowing the DOM standard
ones), not "declare".

I need a vacation.

/be
BAD (script can override getters):

  return contentWindow.document.title == contentWindow.getSelection();

BAD (script can override getters):

  return contentWindow.document.title ==
         contentWindow.__proto__.getSelection.call(contentWindow);

BAD (script can override getters):

  var winWrapper = new XPCNativeWrapper(contentWindow,
                                        'document', getSelection()');
  return winWrapper.document.title == winWrapper.getSelection();

GOOD (gets the real, native getters):

  var winWrapper = new XPCNativeWrapper(contentWindow,
                                        'document', 'getSelection()');
  var docWrapper = new XPCNativeWrapper(winWrapper.document, 'title');
  return docWrapper.title == winWrapper.getSelection();


The above only applies to scripts for Firefox 1.0.2 and earlier.  Scripts
designed to run only in Firefox 1.0.3 and later may simply call:

  return contentWindow.document.title == contentWindow.getSelection();

Scripts that are designed to run in *both* Firefox 1.0.2 and earlier and Firefox
1.0.3 and later are encouraged to use the XPCNativeWrapper version of the script.
(In reply to comment #6)
> Not many extensions should need revision for 1.0.3.  

Translate (http://www.ctomer.com/index.php?cat=3) is one of the ones needing to
be rewritten. It doesn't work anymore with Mozilla/5.0 (Windows; U; Windows NT
5.1; it-IT; rv:1.7.7) Gecko/20050414 Firefox/1.0 :

Errore: uncaught exception: [Exception... "Illegal operation on WrappedNative
prototype object"  nsresult: "0x8057000c (NS_ERROR_XPC_BAD_OP_ON_WN_PROTO)" 
location: "JS frame :: chrome://translate/content/translate.js :: anonymous ::
line 420"  data: no]
(In reply to comment #10)
>It doesn't work anymore with Mozilla/5.0 (Windows; U; Windows NT
> 5.1; it-IT; rv:1.7.7) Gecko/20050414 Firefox/1.0 :

I'm sorry, the right user-agent is: Mozilla/5.0 (Windows; U; Windows NT 5.1;
it-IT; rv:1.7.7) Gecko/20050414 Firefox/1.0.3
Even LinkVisitor doesn't work anymore...

Errore: uncaught exception: [Exception... "Illegal operation on WrappedNative
prototype object"  nsresult: "0x8057000c (NS_ERROR_XPC_BAD_OP_ON_WN_PROTO)" 
location: "JS frame :: chrome://linkvisitor/content/lvbrowseroverlay.js ::
anonymous :: line 144"  data: no]
Flags: blocking-aviary1.0.3?
Bug can be closed. I fixed dictionarysearch :-)
(In reply to comment #13)
> Bug can be closed. I fixed dictionarysearch :-)

https://addons.update.mozilla.org/extensions/moreinfo.php?application=firefox&id=68

Is that the version already on UMO?
This is WFM with the version from comment #14
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.