220215 - use the xpcnativewrapper for the current callers of components.lookupmethod

Status: RESOLVED FIXED

(SeaMonkey :: General, defect)

Description

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment 1

[[e-mail]]

Attachment: Editor portion of patch

Comment 2

[[e-mail]]

Comment on attachment 132135 [details] [diff] [review]
Editor portion of patch

glazou says moa if I test it 10 times but I can only think of 6 tests so far
:-/

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 132135 [details] [diff] [review]
Editor portion of patch

sr=bzbarsky

Comment 4

[[e-mail]]

Attachment: Xpfe portion of patch

Comment 5

[[e-mail]]

There's an inspector use too, although I didn't realize the style property on an
element is replaceable (or is it?)

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 132190 [details] [diff] [review]
Xpfe portion of patch

Hmmm how come just importing the JS in contentAreaContextOverlay makes it
available in all those JS files (eg navigator.js)?  Have you tested all the
callsites?

Comment 7

[[e-mail]]

Attachment: Inspector portion of patch

Comment 8

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 132190 [details] [diff] [review]
Xpfe portion of patch

Does JS Console etc use any of these files, especially contentAreaUtils and
will it care about these functions?  I think editor also uses that file...

Comment 9

[[e-mail]]

Comment on attachment 132190 [details] [diff] [review]
Xpfe portion of patch

mailNavigatorOverlay.xul and navigator.js are only used by navigator, which
already uses contentAreaContextOverlay.xul

contentAreaUtils.js and nsContextMenu.js are only used by
contentAreaContextOverlay.xul which (surprise) is where I am also including
XPCNativeWrapper.js

Sorry for not being more obvious.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 132190 [details] [diff] [review]
Xpfe portion of patch

sr=bzbarsky

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 132190 [details] [diff] [review]
Xpfe portion of patch

>Index: communicator/resources/content/nsContextMenu.js
>+          var spec = new XPCNativeWrapper(window.content.opener, 
"location").location;

accessing .opener is already not safe, no?  The wrapper needs to be on
window.content (this was buggy in the code you changed already).

>+          var characterSet = new XPCNativeWrapper(this.target.ownerDocument, "characterSet").characterSet;

Accessing ownerDocument is already not safe; the wrapper needs to be on
this.target.  Although you may also need to rewrap the ownerDocument object
returned; please test.

>Index: browser/resources/content/navigator.js
>+      var spec = new XPCNativeWrapper(window.content.opener, "location").location;

Accessing opener is unsafe.

> Index: browser/resources/content/mailNavigatorOverlay.xul

>+                                 new XPCNativeWrapper(window._content.document, 'title').title);"/>

Accessing .document is not safe; you want the wrapper on window._content....

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 132345 [details] [diff] [review]
Inspector portion of patch

sr=bzbarsky on this one; the other sr was a case of mistaken patch identity.

Comment 13

[[e-mail]]

Attachment: Improved xpfe portion of patch

I've hacked Send Frame/Link/Page around a bit but they still work.

Comment 14

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 132355 [details] [diff] [review]
Improved xpfe portion of patch

Hm, I'm wondering why I put some of these wrapers on fooWin.location.href... 
That should be completely safe.

See:
http://lxr.mozilla.org/mozilla/source/dom/src/base/nsDOMClassInfo.cpp#3144
http://lxr.mozilla.org/mozilla/source/dom/src/base/nsDOMClassInfo.cpp#3182
http://lxr.mozilla.org/mozilla/source/dom/src/base/nsDOMClassInfo.cpp#3209
http://lxr.mozilla.org/mozilla/source/dom/src/base/nsDOMClassInfo.cpp#4145
etc.

Also, this is kind of obtuse enough, could you please not nest |new
XPCNativeWrapper|s?  Just use a temporary local.  You aren't gaining that much
by not doing that, and you're losing readability.

Comment 15

[[e-mail]]

Attachment: Removed unnecessary wrappers on window.document/.location

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 132543 [details] [diff] [review]
Removed unnecessary wrappers on window.document/.location

>Index: browser/resources/content/navigator.js

>     gFocusedDocument = focusedWindow.document;

Is this safe? sr=me if caillon is happy with it.

Comment 17

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 132543 [details] [diff] [review]
Removed unnecessary wrappers on window.document/.location

>--- browser/resources/content/mailNavigatorOverlay.xul	30 May 2003 05:08:36 -0000	1.4
>+++ browser/resources/content/mailNavigatorOverlay.xul	2 Oct 2003 20:28:46 -0000
>@@ -120,12 +120,10 @@
>     {
>       if (!aDocument)
>         aDocument = window._content.document;

This looks like it needs wrapping.

>-        
>-      var pageUrl = aDocument.URL;
>-      var pageTitle = Components.lookupMethod(aDocument, 'title').call(aDocument);
> 
>       try {
>-        openComposeWindow(pageUrl, pageTitle, 1);
>+        aDocument = new XPCNativeWrapper(aDocument, "URL", "title");
>+        openComposeWindow(aDocument.URL, aDocument.title, 1);
>       } catch(ex) { dump("Cannot Send Page: " + ex + "\n"); }


>--- browser/resources/content/navigator.js	23 Sep 2003 06:49:05 -0000	1.517
>+++ browser/resources/content/navigator.js	2 Oct 2003 20:28:48 -0000
>@@ -271,7 +271,7 @@ function contentAreaFrameFocus()
> {
>   var focusedWindow = document.commandDispatcher.focusedWindow;
>   if (isContentFrame(focusedWindow)) {
>-    gFocusedURL = Components.lookupMethod(focusedWindow, 'location').call(focusedWindow).href;
>+    gFocusedURL = focusedWindow.location.href;
>     gFocusedDocument = focusedWindow.document;

This, too.

Comment 18

[[e-mail]]

Attachment: Actually it's document.location that's safe, window is totally unsafe

Comment 19

[[e-mail]]

Comment on attachment 135169 [details] [diff] [review]
Actually it's document.location that's safe, window is totally unsafe

Re-requesting reviews as the patch is significantly different.

Comment 20

[Boris Zbarsky [:bzbarsky]]

I'll probably take me about a week to get to this.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 135169 [details] [diff] [review]
Actually it's document.location that's safe, window is totally unsafe

sr=bzbarsky

Comment 22

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 135169 [details] [diff] [review]
Actually it's document.location that's safe, window is totally unsafe

Both suck.  r=me if you put back the document change.  I will try hacking
classinfo and caps stuff to see if I can get a better all around solution....

Comment 23

[[e-mail]]

Attachment: wrapping .location again

Comment 24

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 136014 [details] [diff] [review]
wrapping .location again

> 
>-    function sendLink(pageUrl, pageTitle)
>+    function sendLink(aDocument)
>     {
>+      if (!aDocument)
>+        aDocument = window._content.document;

Nit: Use |window.content| instead?  It's slightly faster.

r=me.  I hope to come up with a better generic solution for this whole issue
sometime soon.	It really annoys me that we need to use the XPCWrapper; though
it is better than not, I will agree.

Comment 25

[[e-mail]]

Fix checked in.

Comment 26

[R.K.Aa.]

Sometime between 2004-05-20 09:00:00 and 2004-05-21 08:00:00 "save as" stopped
working fromm the View Source windows. Could this checkin have caused that?
See bug 248964