Bug 465757

Comment 1 Niraj Modi 2015-04-29 05:12:18 EDT

(In reply to Sravan Kumar Lakkimsetti from comment #0)
> Tested this using Snippet308. This particular problem happens with Xulrunner
> 10, xulrunner 24. doesnot happen with Xulrunner 1.9.

Similar behavior on Windows as well, Snippet308 works with xulrunner1.9.2 only and not with xulrunner24, xulrunner10 & xulrunner1.9.1

Comment 2 Lakshmi P Shanmugam 2015-04-29 07:14:20 EDT

(In reply to Niraj Modi from comment #1)
> (In reply to Sravan Kumar Lakkimsetti from comment #0)
> > Tested this using Snippet308. This particular problem happens with Xulrunner
> > 10, xulrunner 24. doesnot happen with Xulrunner 1.9.
> 
> Similar behavior on Windows as well, Snippet308 works with xulrunner1.9.2
> only and not with xulrunner24, xulrunner10 & xulrunner1.9.1

Same problem on Mac too.
Using Browser.evaluate(script, false) works.

Comment 3 Arun Thondapu 2015-04-29 08:04:26 EDT

*** Bug 421905 has been marked as a duplicate of this bug. ***

Comment 4 Matt Painter 2015-04-29 13:41:16 EDT

If you run a script with elevated security in XUL >=24, you will need to use window.document rather than just document, as the script is not run within the context of the window, rather in the context of the chrome.

Comment 5 Matt Painter 2015-04-29 13:46:12 EDT

(In reply to Matt Painter from comment #4)
> If you run a script with elevated security in XUL >=24, you will need to use
> window.document rather than just document, as the script is not run within
> the context of the window, rather in the context of the chrome.

Also, there should be no change in behaviour for XUL<24:

https://github.com/eclipse/eclipse.platform.swt/blob/1343f2a0af658526a93fb8a0e3f0af288c7972f2/bundles/org.eclipse.swt/Eclipse%20SWT%20Mozilla/common/org/eclipse/swt/browser/Mozilla.java#L1399

It was decided that the "trusted" value should default to true, but I was lobbying for it to false by default because it was the most secure and compatible with existing code (e.g. window.document vs document)

Comment 6 Lakshmi P Shanmugam 2015-04-30 06:57:22 EDT

(In reply to Matt Painter from comment #5)
> (In reply to Matt Painter from comment #4)
> > If you run a script with elevated security in XUL >=24, you will need to use
> > window.document rather than just document, as the script is not run within
> > the context of the window, rather in the context of the chrome.
> 
> Also, there should be no change in behaviour for XUL<24:
> 
> https://github.com/eclipse/eclipse.platform.swt/blob/
> 1343f2a0af658526a93fb8a0e3f0af288c7972f2/bundles/org.eclipse.swt/
> Eclipse%20SWT%20Mozilla/common/org/eclipse/swt/browser/Mozilla.java#L1399
> 
> It was decided that the "trusted" value should default to true, but I was
> lobbying for it to false by default because it was the most secure and
> compatible with existing code (e.g. window.document vs document)

The main idea behind having "trusted" = true as the default behavior is to make sure that we don't break the existing client code that uses evaluate() API. 
Existing client code using evaluate can run script with chrome code with XUL < 24. So, the idea was to keep the same behavior for XUL >= 24.
But, I'm not sure if it is right to require that the existing code be modified for evaluate(script) to work.

Comment 7 Matt Painter 2015-04-30 09:38:30 EDT

> The main idea behind having "trusted" = true as the default behavior is to
> make sure that we don't break the existing client code that uses evaluate()
> API. 
> Existing client code using evaluate can run script with chrome code with XUL
> < 24. So, the idea was to keep the same behavior for XUL >= 24.
> But, I'm not sure if it is right to require that the existing code be
> modified for evaluate(script) to work.

IMHO it should be changed so trusted=false by default; the only client code this would require changing is code requiring chrome-level privileges *when* the client app upgrades their XUL version (which as someone with a client app I find reasonable). Most people are just going to be running DOM-level code.

Comment 8 Niraj Modi 2015-05-13 06:49:05 EDT

Proposed approach to the fix looks good.

Comment 9 Lakshmi P Shanmugam 2015-05-13 14:08:43 EDT

Created attachment 253457 [details]
modified evaluate(script)

The javadoc of evaluate(script) mentions that the script is executed in the context of the current document. Modified evaluate(script) so that the behavior is as per javadoc.

Comment 10 Eclipse Genie 2015-05-13 15:09:26 EDT

New Gerrit change created: https://git.eclipse.org/r/47881

Comment 11 Eclipse Genie 2015-05-13 15:09:28 EDT

New Gerrit change created: https://git.eclipse.org/r/47880

Comment 12 Lakshmi P Shanmugam 2015-05-13 15:12:10 EDT

Arun, can you please review the 2 patches.
https://git.eclipse.org/r/47881 is to fix the issue for versions < 24.
https://git.eclipse.org/r/47881 is to fix the issue for versions >= 24.

Comment 13 Eclipse Genie 2015-05-13 15:19:04 EDT

New Gerrit change created: https://git.eclipse.org/r/47883

Comment 14 Lakshmi P Shanmugam 2015-05-13 15:20:59 EDT

(In reply to Lakshmi Shanmugam from comment #12)
> Arun, can you please review the 2 patches.
> https://git.eclipse.org/r/47881 is to fix the issue for versions < 24.
> https://git.eclipse.org/r/47881 is to fix the issue for versions >= 24.
Correction
https://git.eclipse.org/r/47880 is to fix the issue for versions < 24.
https://git.eclipse.org/r/47883 is to fix the issue for versions >= 24.

Comment 15 Arun Thondapu 2015-05-13 15:40:34 EDT

(In reply to Lakshmi Shanmugam from comment #14)
> (In reply to Lakshmi Shanmugam from comment #12)
> > Arun, can you please review the 2 patches.
> > https://git.eclipse.org/r/47881 is to fix the issue for versions < 24.
> > https://git.eclipse.org/r/47881 is to fix the issue for versions >= 24.
> Correction
> https://git.eclipse.org/r/47880 is to fix the issue for versions < 24.
> https://git.eclipse.org/r/47883 is to fix the issue for versions >= 24.

I've reviewed them and both patches look good for merging.

Comment 16 Eclipse Genie 2015-05-13 15:49:00 EDT

Gerrit change https://git.eclipse.org/r/47880 was merged to [master].
Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=6b28578ec575cf9b9bc42578b2cd2a249f83e1ad

Comment 17 Eclipse Genie 2015-05-13 15:55:54 EDT

Gerrit change https://git.eclipse.org/r/47883 was merged to [master].
Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=6a69a1fd95a7e88062f2270d5c638a035c824208

Comment 18 Lakshmi P Shanmugam 2015-05-14 08:05:34 EDT

(In reply to Arun Thondapu from comment #15)
> (In reply to Lakshmi Shanmugam from comment #14)
> > (In reply to Lakshmi Shanmugam from comment #12)
> > > Arun, can you please review the 2 patches.
> > > https://git.eclipse.org/r/47881 is to fix the issue for versions < 24.
> > > https://git.eclipse.org/r/47881 is to fix the issue for versions >= 24.
> > Correction
> > https://git.eclipse.org/r/47880 is to fix the issue for versions < 24.
> > https://git.eclipse.org/r/47883 is to fix the issue for versions >= 24.
> 
> I've reviewed them and both patches look good for merging.

Thanks Arun!
I've verified the fix with I20150513-2000 on Mac and XULRunner 24 & 31.

Comment 19 Sravan Kumar Lakkimsetti 2015-05-14 09:34:03 EDT

I've verified the fix with I20150513-2000 on linux with xulrunner 10 and 24