Last Comment Bug 290200 - dictionary search extension broken in latest nightlies
: dictionary search extension broken in latest nightlies
Status: RESOLVED WORKSFORME
:
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: 1.0 Branch
: x86 All
-- normal (vote)
: ---
Assigned To: Asa Dotzler [:asa]
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2005-04-13 12:12 PDT by Chase Phillips
Modified: 2006-02-11 23:20 PST (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments

Description User image Chase Phillips 2005-04-13 12:12:03 PDT
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.
Comment 1 User image Chase Phillips 2005-04-13 12:12:57 PDT
Nominating blocking-aviary1.0.3.
Comment 2 User image Asa Dotzler [:asa] 2005-04-13 12:54:55 PDT
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
Comment 3 User image Asa Dotzler [:asa] 2005-04-13 13:03:10 PDT
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();
    }
Comment 4 User image Brendan Eich [:brendan] 2005-04-13 13:09:35 PDT
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
Comment 5 User image Daniel Veditz [:dveditz] 2005-04-13 14:11:56 PDT
(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?
Comment 6 User image Brendan Eich [:brendan] 2005-04-13 14:23:15 PDT
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
Comment 7 User image Brendan Eich [:brendan] 2005-04-13 14:27:48 PDT
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
Comment 8 User image Brendan Eich [:brendan] 2005-04-13 14:30:08 PDT
> (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
Comment 9 User image Christopher Aillon (sabbatical, not receiving bugmail) 2005-04-13 17:41:32 PDT
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.
Comment 10 User image Giuliano Masseroni [:jooliaan] 2005-04-15 06:04:12 PDT
(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]
Comment 11 User image Giuliano Masseroni [:jooliaan] 2005-04-15 06:11:23 PDT
(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
Comment 12 User image Giuliano Masseroni [:jooliaan] 2005-04-15 06:30:27 PDT
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]
Comment 13 User image Jaap A. Haitsma 2005-04-19 13:37:44 PDT
Bug can be closed. I fixed dictionarysearch :-)
Comment 14 User image alanjstr 2005-04-19 14:32:50 PDT
(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?
Comment 15 User image Robert Strong [:rstrong] (use needinfo to contact me) 2006-02-11 23:20:40 PST
This is WFM with the version from comment #14

Note You need to log in before you can comment on or make changes to this bug.