Closed
Bug 844406
Opened 12 years ago
Closed 12 years ago
Greasemonkey script stops working correctly in Firefox 20. 'self-hosted' builtin functions destroy js-callstack in certain case.
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: mjh563, Assigned: till)
References
Details
(Keywords: addon-compat, regression)
Attachments
(6 files, 1 obsolete file)
(deleted),
text/javascript
|
Details | |
(deleted),
text/plain
|
Details | |
(deleted),
application/octet-stream
|
Details | |
(deleted),
text/plain
|
Details | |
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
After upgrading to Firefox 20, one of my Greasemonkey scripts stopped working. The attached file is a reduced testcase that demonstrates the problem. This script alerts "Success" in Firefox 19 but "Fail" in Firefox 20.
Steps to Reproduce:
1. Install Greasemonkey from https://addons.mozilla.org/en-US/firefox/addon/greasemonkey/
2. Install the script by clicking on the attached file
3. Load http://www.ebay.co.uk
Actual results:
1. An alert box shows "Fail"
2. In the Error Console: "Greasemonkey access violation: unsafeWindow cannot call GM_getResourceText."
Expected results:
An alert box shows "Success"
Regression range is
Last good nightly: 2012-11-22
First bad nightly: 2012-11-23
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=20ec9014f220&tochange=d8e4f06198dc
Attachment #717457 -
Attachment mime type: application/octet-stream → text/javascript
Hmm, Step 2 of the STR doesn't seem to work. I think that's because Greasemonkey doesn't recognise the URL.
Revised Step 2:
Save the attachment as "Testcase script.user.js", then open that file in Firefox. A Greasemonkey installation window should appear when you do that.
Comment 3•12 years ago
|
||
Regression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/fcad43f8558f
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20.0 Firefox/20.0 ID:20121122010354
Bad:
http://hg.mozilla.org/mozilla-central/rev/3b71d63eafd5
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20.0 Firefox/20.0 ID:20121122030852
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=fcad43f8558f&tochange=3b71d63eafd5
Regression window(m-i)
Good:http://hg.mozilla.org/integration/mozilla-inbound/rev/7ae5ff606cf2
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20.0 Firefox/20.0 ID:20121121153752
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/37c36aa6f563
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20.0 Firefox/20.0 ID:20121121165251
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=7ae5ff606cf2&tochange=37c36aa6f563
Triggered by:
5593cd83590e Till Schneidereit — Bug 784294 - Convert some array extras to self-hosted js implementations. r=Waldo The following methods are converted: - lastIndexOf - indexOf - forEach - some - every - reduce - reduceRight
Assignee: nobody → general
Blocks: 784294
status-firefox19:
--- → unaffected
tracking-firefox20:
--- → ?
tracking-firefox21:
--- → ?
tracking-firefox22:
--- → ?
Component: Untriaged → JavaScript Engine
Keywords: regression
OS: Linux → All
Product: Firefox → Core
Updated•12 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee | ||
Comment 4•12 years ago
|
||
Looking into it now
Assignee: general → tschneidereit
Status: NEW → ASSIGNED
Assignee | ||
Comment 5•12 years ago
|
||
So this is pretty weird: invoked from inside Array#forEach, GM_getResourceText seems to lose access to its environment. Or something.
Unfortunately, GreaseMonkey swallows all errors in scripts, so I don't know what goes on, internally.
I suspect that the problem is connected to scripts being executed in sandboxes, but I don't yet know more than that.
Attachment #717457 -
Attachment is obsolete: true
Comment 6•12 years ago
|
||
Scriptish[1] also fails. ([1]: https://addons.mozilla.org/en-US/firefox/addon/scriptish/ )
Comment 7•12 years ago
|
||
The problem is that js callstack is different after landing Bug 784294. Then Greasemonkey throw an error.
e.g., Evaluate the following code in Scratchpad(Chrome privilege):
function a(){
let stack = Components.stack;
do {
Components.classes["@mozilla.org/consoleservice;1"]
.getService(Components.interfaces.nsIConsoleService)
.logStringMessage(stack.language +" "+stack.filename);
} while (stack = stack.caller);
return true;
}
[1].forEach(function() {
a();
});
Before landing Bug 784294,
2 Scratchpad/1
2 Scratchpad/1
2 Scratchpad/1
0 null
After landing Bug 784294
2 Scratchpad/1
2 Scratchpad/1
2 self-hosted
2 Scratchpad/1
0 null
Comment 8•12 years ago
|
||
So, I think that this is not Firefox bug,
Greasemonkey/Scriptish should modify their code,
Comment 9•12 years ago
|
||
Jorge: can you reach out to the people at Greasemonkey/Scriptish?
Tracking for now so we can keep this on our radar. If the fix is external, we'll remove tracking once contact has been made.
Reporter | ||
Comment 10•12 years ago
|
||
This is already logged on the Greasemonkey issue tracker; see
https://github.com/greasemonkey/greasemonkey/issues/1715
Updated•12 years ago
|
Keywords: addon-compat
Comment 11•12 years ago
|
||
As this is being tracked on greasemonkey's issue tracker (and there appears to be a fix over there) un-tracking here.
Comment 12•12 years ago
|
||
Can anyone more knowledgeable let me know what the implications of this are?
Today, Greasemonkey (sometimes) exposes privileged APIs to user scripts. When they are called, the call stack is inspected to guarantee that only chrome + user scripts are on the stack, to ensure that content code is not gaining access to these privileged methods. (It's possible for a malicious/poorly written script to leak references to them to content.)
I don't quite know what self-hosted really means. Is it actually safe to assume that "self hosted" is not content-controlled, and thus possibly malicious?
Comment 13•12 years ago
|
||
I've taken a closer look, and logged details in the linked Greasemonkey issue.
This change seems to make our security checks impossible; "self-hosted" code from the user script sandbox and from content cannot be disambiguated by examining the stack.
Is there perhaps a better way to implement this sort of check (JS caller is trusted to do X only if it comes from the sandbox we create and not from content)?
Comment 14•12 years ago
|
||
How do I set this to tracking for 20 again? This seems to be a big breaking change, at least from Greasemonkey's perspective. The "workaround" originally reported at Greasemonkey's issue is not appropriate: it just disables our security checks.
Comment 15•12 years ago
|
||
(In reply to Anthony Lieuallen from comment #13)
> This change seems to make our security checks impossible; "self-hosted" code
> from the user script sandbox and from content cannot be disambiguated by
> examining the stack.
Could you please attach testcase?
Comment 16•12 years ago
|
||
It's a difficult test case to make, so the steps to reproduce are a bit ugly. But here's what I've got for now:
Install this patched version of Greasemonkey which logs (to the console, via dump()), its activity in apiLeakCheck, and never returns false (which would stop the checks):
https://dl.dropbox.com/u/6784911/greasemonkey-1.8.b844406.xpi
Then install this user script:
https://gist.github.com/arantius/5078045
Then load this page:
https://dl.dropbox.com/u/6784911/b844406.html
In firefox 19, the console displays:
calling GM_getValue ...
>>> apiLeakCheck ...
resource://greasemonkey/util/apiLeakCheck.js
chrome://greasemonkey/content/miscapis.js
resource://greasemonkey/util/hitch.js
file:///.../Firefox/Profiles/w1ajyiyj.test/gm_scripts/API_Leaker/b844406.user.js
<<< apiLeakCheck
exporting GM_getValue
>>> apiLeakCheck ...
resource://greasemonkey/util/apiLeakCheck.js
chrome://greasemonkey/content/miscapis.js
resource://greasemonkey/util/hitch.js
https://dl.dropbox.com/u/6784911/b844406.html
Would have blocked here!
<<< apiLeakCheck
That is: the user script itself calls GM_getValue once, and apiLeakCheck logs the stack that it sees: greasemonkey itself, and the user script inside the profile gm_scripts directory. Then it leaks the reference to this function to the content scope. The content scope calls, with a funky syntax, this leaked method. The apiLeakCheck sees the content scope in the stack: the last "https://" URL, and logs that it would have (patch does not actually) blocked the call, here.
Repeat that with Firefox 20, and the console shows:
calling GM_getValue ...
>>> apiLeakCheck ...
resource://greasemonkey/util/apiLeakCheck.js
chrome://greasemonkey/content/miscapis.js
resource://greasemonkey/util/hitch.js
file:///.../Firefox/Profiles/w1ajyiyj.test/gm_scripts/API_Leaker/b844406.user.js
<<< apiLeakCheck
exporting GM_getValue
>>> apiLeakCheck ...
resource://greasemonkey/util/apiLeakCheck.js
chrome://greasemonkey/content/miscapis.js
resource://greasemonkey/util/hitch.js
self-hosted
Would have blocked here!
self-hosted
Would have blocked here!
<<< apiLeakCheck
The stack when the script itself calls the method is unchanged. However, when content calls the leaked reference, the content-scoped filename property is gone, now only "self-hosted" is displayed. If the same weird syntax is used inside a script, as in the content-hosted example, then the stack trace looks exactly the same as the call from content: they cannot be disambiguated. We cannot selectively allow scripts running in our sandbox to call the function, and block leaked references to content from being used.
(And given the design of Greasemonkey, we cannot make it impossible for references to leak. To be useful, scripts need a reference to content, to alter it. And lots of scripts are poorly written, and make leaks like this unintentionally. Or advanced attackers may be able to abuse even more benign accesses to content. So we have to do this sort of check.)
Comment 17•12 years ago
|
||
Comment 18•12 years ago
|
||
Comment 19•12 years ago
|
||
Comment 20•12 years ago
|
||
Comment 21•12 years ago
|
||
(In reply to Anthony Lieuallen from comment #16)
> The stack when the script itself calls the method is unchanged. However,
> when content calls the leaked reference, the content-scoped filename
> property is gone, now only "self-hosted" is displayed.
I van confirmed. Missing js callstak of the content-scoped filename.
And I request to track for Firefox 20 and later.
Updated•12 years ago
|
Summary: Greasemonkey script stops working correctly in Firefox 20 → Greasemonkey script stops working correctly in Firefox 20. 'self-hosted' builtin functions destroy js-callstack in certain case.
Comment 22•12 years ago
|
||
Till - can you enumerate the options here? We'll be going to build with FF20 beta 3 tomorrow and will only consider low-risk, speculative fixes in the next week (up to beta 4) after that do we have the option of backing out the changes in bug 784294 to not ship this regression in FF20? Or other potential remedies?
Flags: needinfo?(tschneidereit)
Assignee | ||
Comment 23•12 years ago
|
||
Argh, I'm sorry for dropping the ball on this bug. My bugmail filters clearly don't always work as intended.
The "self-hosted" entry shouldn't be contained in the stack, so this definitely is a regression from self-hosting. I'll bet it's caused by the same issue as bug 787927, so I'll investigate that.
In short, it seems like we're not always setting the "self-hosted" flag on functions when cloning them, which is why this problem only occurs in scenarios that involve sandboxes.
I'm hopeful that a solution isn't going to be risky: it should consist of correctly copying the state of a flag during a cloning path where that doesn't yet happen.
In case the solution turns out to be more risky, backing out bug 784294 would be an option, yes. It's a completely self-contained change and should be easy to back out.
Flags: needinfo?(tschneidereit)
Comment 24•12 years ago
|
||
Given the feedback in comment 23 and the fact that we're approaching beta 4, let's go ahead with backing out bug 784294 for FF20 and continue to track this for a proper fix later.
Assignee | ||
Comment 25•12 years ago
|
||
I'd very much like to instead check in the fix for bug 787927 instead. That is very low-risk and fixes this issue, too. Will formally request uplift once I get a review.
Depends on: 787927
Assignee | ||
Comment 26•12 years ago
|
||
Fixed through fixing bug 787927
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 27•12 years ago
|
||
Verified fixed with Firefox 20 beta 5, on Windows XP 32-bit, build ID: 20130313170052. After following the STR from the description and comment 2, I get an alert box that shows "Success".
Also, bug 787927 is verified too with the beta 5 build.
QA Contact: manuela.muntean
Comment 28•12 years ago
|
||
Verified also on Mac OSX 10.7.5 and Ubuntu 12.04 32-bit, with Firefox 20 beta 5.
Comment 29•12 years ago
|
||
I've just updated Firefox 20/beta channel (I still don't know how to communicate which build I'm actually using now besides "beta channel on such-and-such date"). What I see in the console when I execute my test case from comment 16 is:
calling GM_getValue ...
>>> apiLeakCheck ...
resource://greasemonkey/util/apiLeakCheck.js
chrome://greasemonkey/content/miscapis.js
resource://greasemonkey/util/hitch.js
file:///C:/Users/t-bone/AppData/Roaming/Mozilla/Firefox/Profiles/w1ajyiyj.test/g
m_scripts/API_Leaker/b844406.user.js
<<< apiLeakCheck
exporting GM_getValue
>>> apiLeakCheck ...
resource://greasemonkey/util/apiLeakCheck.js
chrome://greasemonkey/content/miscapis.js
resource://greasemonkey/util/hitch.js
self-hosted
Would have blocked here!
<<< apiLeakCheck
Or: the call from the sandbox identifies itself as such, so the basic case (detect in-sandbox execution) generally works. But the content-scoped call still says "self-hosted" in the stack trace. And only once. Is that expected?
Comment 30•12 years ago
|
||
I can confirm that the problem is still reproducible in Firefox20.0b5.
http://hg.mozilla.org/releases/mozilla-beta/rev/163304f85fc1
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20100101 Firefox/20.0 ID:20130313170052
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to Anthony Lieuallen from comment #29)
> I've just updated Firefox 20/beta channel (I still don't know how to
> communicate which build I'm actually using now besides "beta channel on
> such-and-such date").
Go to about:buildconfig and click the "Built from" link, which should take you to a page on hg.mozilla.org. Somewhere near the top of the page should be a yellow box saying "FIREFOX_20_0b5_RELEASE".
Comment 32•12 years ago
|
||
'self-hosted' built-in functions still destroy js-callstack in certain case.
Steps to Reproduce:
Evaluate the following code in Scratchpad(Chrome privilege):
window.a = function a(){
let stack = Components.stack;
do {
Components.classes["@mozilla.org/consoleservice;1"]
.getService(Components.interfaces.nsIConsoleService)
.logStringMessage(stack.language +" "+stack.filename);
} while (stack = stack.caller);
return true;
};
['["test"].forEach(a);'].forEach(setTimeout);
Expected Results Firefox19.0.2 (before landing Bug 784294):
2 Scratchpad/1
2 Scratchpad/1
0 null
Actual Results Firefox20.0b5(after landing Bug 784294 and Bug 787927):
2 Scratchpad/1
2 self-hosted
0 null
Updated•12 years ago
|
Comment 33•12 years ago
|
||
@Till,
I think that it is time to back bug784294 out because Firefox20 will be released soon.
Comment 34•12 years ago
|
||
> Expected Results Firefox19.0.2
> 2 Scratchpad/1
> 2 Scratchpad/1
> 0 null
>
> Actual Results Firefox20.0b5
> 2 Scratchpad/1
> 2 self-hosted
> 0 null
I was also able to reproduce these errors, on an Ubuntu 32-bit machine, with the builds specified above.
Assignee | ||
Comment 35•12 years ago
|
||
[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 784294, bug 787927
User impact if declined: Some addons (especially GreaseMonkey) fail, plus some top-crashers caused by the attempted fix in bug 787927 (see bug 851788)
Testing completed (on m-c, etc.): this is a beta-specific backout
Risk to taking this patch (and alternatives if risky): none, straight reversal to previous implementation of the Array extras
String or UUID changes made by this patch: none
I will work on fixing the regression for Aurora, but I agree that not backing out 784294 is too risky at this point.
Attachment #725891 -
Flags: approval-mozilla-beta?
Updated•12 years ago
|
Comment 36•12 years ago
|
||
Comment on attachment 725891 [details] [diff] [review]
Backout of self-hosted array extras from beta
Thanks for preparing this backout, please land asap (before tomorrow morning) so that we can make sure to collect 20.0b5 feedback (and crash volume) to confirm this works as intended.
Attachment #725891 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Assignee | ||
Comment 37•12 years ago
|
||
Updated•12 years ago
|
Comment 38•12 years ago
|
||
Firefox 20 beta 6 - build ID: 20130320062118 - Ubuntu 12.04 32-bit
1) After following the STR from comment 32, I don't get those results anymore; all I get is:
Timestamp: 03/21/2013 10:54:44 AM
Error: The Components object is deprecated. It will soon be removed.
Source File: Scratchpad/1
Line: 11
Timestamp: 03/21/2013 10:54:44 AM
Error: Error: Permission denied to access property 'stack'
Source File: Scratchpad/1
Line: 11
2) After following the STR from the description and comment 2, I get an alert box that shows "Success".
Comment 39•12 years ago
|
||
Firefox 20 beta 6 - build ID: 20130320062118 - Mac OSX 10.7.5
1) After following the STR from comment 32, I get the expected results:
2 Scratchpad/1
2 Scratchpad/1
0 null
2) After following the STR from the description and comment 2, I get an alert box that shows "Success".
Firefox 20 beta 6 - build ID: 20130320062118 - Windows XP
1) After following the STR from comment 32, all I get is:
Error: Error: Permission denied to access property 'stack'
Source File: Scratchpad/1
Line: 11
2) After following the STR from the description and comment 2, I get an alert box that shows "Success".
Verified fixed based on comment 38 and comment 39.
Comment 40•12 years ago
|
||
In Firefox Nightly 22.0a1 (2013-03-21) I am seeing no change since comment 29. Please use those STR; the original report's STR do not exercise the true bug.
The stack trace still shows an entry whose filename is "self-hosted", in the called-from-content case. This should not be so, right?
(p.s. Where do I get beta 6 from? my beta channel install says it's b5 and up to date.)
Comment 41•12 years ago
|
||
(In reply to Anthony Lieuallen from comment #40)
> (p.s. Where do I get beta 6 from? my beta channel install says it's b5 and
> up to date.)
ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/20.0b6-candidates/build1/
Comment 42•12 years ago
|
||
(In reply to Anthony Lieuallen from comment #40)
> In Firefox Nightly 22.0a1 (2013-03-21) I am seeing no change since comment
> 29. Please use those STR; the original report's STR do not exercise the
> true bug.
>
> The stack trace still shows an entry whose filename is "self-hosted", in the
> called-from-content case. This should not be so, right?
With Firefox 20 beta 6, on Ubuntu 12.04 32-bit, after using the STR from comment 32, I don't get an entry whose filename is "self-hosted", in the Error Console (Tools -> Web Developer -> Error Console).
Do you still reproduce the issue with Firefox 20 beta 6? Which OS are you using?
Comment 43•12 years ago
|
||
> With Firefox 20 beta 6, on Ubuntu 12.04 32-bit, after using the STR from
> comment 32, I don't get an entry whose filename is "self-hosted", in the
> Error Console (Tools -> Web Developer -> Error Console).
Sorry, I meant comment 29, not 32.
Assignee | ||
Comment 44•12 years ago
|
||
The problem isn't really a self-hosted function bein on the stack. Instead, setTimeout stores the calling function's filename and line / column number. I have a patch fixing this in bug 853417, which I hope to uplift to Aurora once it's pushed to central.
Here's a much simpler set of STR:
1. run this code in the console or scratchpad (or a website):
["console.log((new Error).fileName)"].forEach(setTimeout);
Status: REOPENED → ASSIGNED
Comment 45•12 years ago
|
||
(In reply to Till Schneidereit [:till] from comment #44)
> The problem isn't really a self-hosted function bein on the stack. Instead,
> setTimeout stores the calling function's filename and line / column number.
> I have a patch fixing this in bug 853417, which I hope to uplift to Aurora
> once it's pushed to central.
>
> Here's a much simpler set of STR:
>
> 1. run this code in the console or scratchpad (or a website):
>
> ["console.log((new Error).fileName)"].forEach(setTimeout);
With Firefox 20 beta 6 on Ubuntu 12.04 32-bit, the Error Console doesn't show any errors, and the Web Console shows this:
[17:06:15.199] Scratchpad/1 Scratchpad/1:10
Comment 46•12 years ago
|
||
With the latest Nightly (build ID: 20130321030940), on the same machine (Ubuntu), the Web Console shows:
[17:14:05.702] self-hosted self-hosted:348
Till, do you get the same results?
Updated•12 years ago
|
Assignee | ||
Comment 47•12 years ago
|
||
> Till, do you get the same results?
Yes, I do. The bug was fixed on beta by backing out all self-hosted code. For Aurora and mozilla-central, it will be fixed by pushing the patch in bug 853417, so those are currently still affected.
Updated•12 years ago
|
Comment 48•12 years ago
|
||
Thanks, sorry for confusion. With 20b6 I indeed see the content URL as expected, in my steps from 16. Confirmed fixed. I was using 20b5 earlier.
I'll go CC myself on bug 853417 so I can watch that fix to post-20 Firefoxen.
Assignee | ||
Comment 49•12 years ago
|
||
Underlying problem is fixed, so this should reproduce in the next Nightly, anymore. Requesting uplift to Aurora now.
Assignee | ||
Comment 50•12 years ago
|
||
(In reply to Till Schneidereit [:till] from comment #49)
> Underlying problem is fixed, so this should reproduce in the next Nightly,
> anymore. Requesting uplift to Aurora now.
... and I forgot actually doing that after some details held up the central. Requested beta uplift now.
Comment 51•12 years ago
|
||
(In reply to Till Schneidereit [:till] from comment #50)
> (In reply to Till Schneidereit [:till] from comment #49)
> > Underlying problem is fixed, so this should reproduce in the next Nightly,
> > anymore. Requesting uplift to Aurora now.
>
> ... and I forgot actually doing that after some details held up the central.
> Requested beta uplift now.
853417, has been approved on beta now in favour of this and help fix top-crasher(Bug 851788).
Requesting QA verification once Fx 21 Beta 2 builds are available .
Updated•12 years ago
|
Comment 52•12 years ago
|
||
Verified fixed with Firefox 21 beta 2 (build ID: 20130408165307), on Mac OSX 10.7.5 and Ubuntu 12.04 32-bit, using the STR from comment 44.
Assignee | ||
Comment 53•12 years ago
|
||
This is fixed on trunk, too. Do I need to take additional steps for that to be verified, or can I close the bug now?
Flags: needinfo?(bbajaj)
Comment 54•12 years ago
|
||
(In reply to Till Schneidereit [:till] from comment #53)
> This is fixed on trunk, too. Do I need to take additional steps for that to
> be verified, or can I close the bug now?
Based on QA verification in comment# 52, you can go ahead and close the bug.
Flags: needinfo?(bbajaj)
Assignee | ||
Comment 55•12 years ago
|
||
Thanks for the info!
Status: ASSIGNED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
Comment 56•12 years ago
|
||
Verified fixed with Firefox 22 beta 2 (build ID: 20130514181517), on Mac OSX 10.8.3, Ubuntu 12.10 32-bit and Windows Vista 32-bit, using the STR from comment 44.
You need to log in
before you can comment on or make changes to this bug.
Description
•