Last Comment Bug 418356 - [FIX]It's unsafe to use mozIJSSubScriptLoader.loadSubScript() with non-chrome urls or chrome urls whose scheme/host part contain uppercase characters
: [FIX]It's unsafe to use mozIJSSubScriptLoader.loadSubScript() with non-chrome...
Status: RESOLVED FIXED
[sg:critical]requires vulnerable addo...
: addon-compat, dev-doc-complete, verified1.8.1.15
Product: Core
Classification: Components
Component: Security (show other bugs)
: Trunk
: x86 Windows XP
: P1 normal (vote)
: mozilla1.9beta5
Assigned To: Johnny Stenback (:jst, jst@mozilla.com)
:
Mentors:
Depends on: 424484 423727 427902
Blocks: 449673
  Show dependency treegraph
 
Reported: 2008-02-18 23:27 PST by moz_bug_r_a4
Modified: 2009-02-16 17:44 PST (History)
33 users (show)
mbeltzner: blocking1.9+
dveditz: blocking1.8.1.15+
dveditz: wanted1.8.1.x+
bzbarsky: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase 2 - test extension (1.83 KB, application/x-xpinstall)
2008-02-18 23:36 PST, moz_bug_r_a4
no flags Details
testcase 2 - html (245 bytes, text/html)
2008-02-18 23:37 PST, moz_bug_r_a4
no flags Details
testcase 2 - test extension v0.2 (1.88 KB, application/x-xpinstall)
2008-02-19 23:15 PST, moz_bug_r_a4
no flags Details
Fix for both issues (5.36 KB, patch)
2008-03-17 19:13 PDT, Boris Zbarsky [:bz]
jst: review+
jst: superreview+
Details | Diff | Splinter Review
I think this might be more viable (3.98 KB, patch)
2008-03-18 23:10 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review
Permit file:// URIs (3.07 KB, patch)
2008-03-20 18:28 PDT, Johnny Stenback (:jst, jst@mozilla.com)
no flags Details | Diff | Splinter Review
Updated fix. (4.12 KB, patch)
2008-03-20 21:34 PDT, Johnny Stenback (:jst, jst@mozilla.com)
bzbarsky: review+
bzbarsky: superreview+
Details | Diff | Splinter Review
1.8 branch version. (7.29 KB, patch)
2008-06-04 12:58 PDT, Johnny Stenback (:jst, jst@mozilla.com)
bzbarsky: review+
bzbarsky: superreview+
dveditz: approval1.8.1.15+
asac: approval1.8.0.next+
Details | Diff | Splinter Review

Description moz_bug_r_a4 2008-02-18 23:27:10 PST
It's unsafe to use mozIJSSubScriptLoader.loadSubScript() with non-chrome urls
or chrome urls whose scheme/host part contain uppercase characters.  Scripts
that are loaded in this way do not use implicit XPCNativeWrappers when
accessing content.

  loader.loadSubScript("chrome://test/..."); // safe
  loader.loadSubScript("chrome://TEST/..."); // unsafe
  loader.loadSubScript("file://...");        // unsafe

Google Toolbar uses mozIJSSubScriptLoader.loadSubScript() with file: url, and
allows an attacker to run arbitrary code with chrome privileges.
Comment 1 moz_bug_r_a4 2008-02-18 23:28:48 PST
Created attachment 304165 [details]
testcase 1 - Arbitrary code execution

This testcase does not work on "https:"//bugzilla.mozilla.org/.  Please set up
this testcase on other server with "http:".

This works on fx2.0.0.x with Google Toolbar.

http://toolbar.google.com/
Comment 2 moz_bug_r_a4 2008-02-18 23:36:11 PST
Created attachment 304166 [details]
testcase 2 - test extension
Comment 3 moz_bug_r_a4 2008-02-18 23:37:15 PST
Created attachment 304167 [details]
testcase 2 - html

Steps to reproduce:
1. Install "test extension".
2. Click "test" button on the top of the browser's toolbox.

Actual Results:
chrome://lss-test/content/sub.js
 -> [object XPCNativeWrapper [object HTMLDocument]] / #document

chrome://LSS-TEST/content/sub.js
 -> [object HTMLDocument] / xxx

CHROME://lss-test/content/sub.js
 -> [object HTMLDocument] / xxx

file://...
 -> [object HTMLDocument] / xxx

data:text/plain,...
 -> [object HTMLDocument] / xxx
Comment 4 Boris Zbarsky [:bz] 2008-02-19 09:01:14 PST
Hmm.  The subscript loader always evaluates the script with the system principal?

Note that we had a similar issue in the component loader; see http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/js/src/xpconnect/loader/mozJSComponentLoader.cpp&rev=1.157#1236  We could probably just copy this code, right?

I'm not sure what the deal with case is; that's likely to be an issue for chrome in general, not just for the subscript loader.  Benjamin might know what's going on there.
Comment 5 moz_bug_r_a4 2008-02-19 23:12:59 PST
(In reply to comment #4)
> Hmm.  The subscript loader always evaluates the script with the system
> principal?
> 

Yes.  I'll attach an updated version of test extension, which also shows
whether the subscript is executed with the system principal.

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/js/src/xpconnect/loader/mozJSSubScriptLoader.cpp&rev=1.25&mark=272,282-283#272
Comment 6 moz_bug_r_a4 2008-02-19 23:15:08 PST
Created attachment 304418 [details]
testcase 2 - test extension v0.2

sub.js:
function x(w) {
	var s;
	try { s = Components.stack; }
	catch (e) { s = e; }
	var d = w.content.document;
	return d.toString() + " / " + d.nodeName + "\n " + s;
}

Actual Results:
chrome://lss-test/content/sub.js
 -> [object XPCNativeWrapper [object HTMLDocument]] / #document
 JS frame :: chrome://lss-test/content/sub.js :: x :: line 4

chrome://LSS-TEST/content/sub.js
 -> [object HTMLDocument] / xxx
 JS frame :: chrome://LSS-TEST/content/sub.js :: x :: line 4

CHROME://lss-test/content/sub.js
 -> [object HTMLDocument] / xxx
 JS frame :: CHROME://lss-test/content/sub.js :: x :: line 4

file://...
 -> [object HTMLDocument] / xxx
 JS frame :: file://.../sub.js :: x :: line 4

data:text/plain,..
 -> [object HTMLDocument] / xxx
 JS frame :: data:text/plain,... :: x :: line 4
Comment 7 Mike Beltzner [:beltzner, not reading bugmail] 2008-02-21 14:21:33 PST
Not going to block for now as this isn't a regression, but that will change if we decide to block 1.8.0.13 for this.
Comment 8 Daniel Veditz [:dveditz] 2008-02-29 11:54:53 PST
Re-requesting trunk blocking. It may not be that common use in add-ons, but with Google Toolbar being vulnerable it's common enough.

Blocking 1.8.1.13 is probably not realistic given the time remaining, but this is a branch blocker as soon as we can get a fix.
Comment 9 Johnny Stenback (:jst, jst@mozilla.com) 2008-03-12 17:56:38 PDT
I don't really see a good way to fix this. The one way we *could* fix this is to make it so that subscripts loaded this way gets implicit wrapper automation if the caller of loadSubScript() is flagged to get implicit wrapper automation. The only sane way I see us being able to do that w/o some really major surgery to the code behind this is to lie and tell the JS engine that the filename of the script we're executing is the caller scripts filename, or at least a name that's prefixed with the callers script filename. I hacked that up, and it works, I made the script filename be "caller-script-filename -> loaded-subscript-uri".

Too hacky? Other thoughts?
Comment 10 Boris Zbarsky [:bz] 2008-03-12 21:01:33 PDT
We could also always wrapper-automate things loaded via the subscript loader...  Just like we do JS components.
Comment 11 Johnny Stenback (:jst, jst@mozilla.com) 2008-03-12 23:09:00 PDT
I don't know that that's safe, that could leak XPCNativeWrappers out to untrusted code, which is bad. IOW, if an extension loads "http://evil.com/foo.js" (or file://, or data:...), and a webpage loads the same JS file, the script when loaded from the webpage will get implicit XPCNativeWrappers, and could then mutate them and fool chrome...
Comment 12 Boris Zbarsky [:bz] 2008-03-12 23:32:36 PDT
Oh, ugh.

How about prohibiting the subscript loader from loading non-chrome scripts, then?    That would make a lot more sense to me anyway; running anything like http://whatever as system script is beyond scary.
Comment 13 Mike Shaver (:shaver -- probably not reading bugmail closely) 2008-03-13 06:51:23 PDT
> How about prohibiting the subscript loader from loading non-chrome scripts,
> then?

Yes please, very much.
Comment 14 Bob Clary [:bc:] 2008-03-13 06:59:17 PDT
I used to use scope.eval to compile scripts into a specific scope, then fell back to using the subscript loader when eval was restricted. Now it appears I won't be able to use the subscript loader. What other methods are available for loading script source and compiling it into a specific scope?
Comment 15 Mike Shaver (:shaver -- probably not reading bugmail closely) 2008-03-13 07:21:23 PDT
Load it into an iframe and it gets a new scope.  Or you can use the subscript loader as long as your script comes from chrome.
Comment 16 Johnny Stenback (:jst, jst@mozilla.com) 2008-03-17 16:30:41 PDT
Just restricting loads to chrome:// here won't solve all the problems in this bug, we still need a way to deal with the package case mismatch issue as well, not sure what's the best way to deal with that yet. If we can assume package names are always lowercased, we could lowercase it here, but I don't know if that's a safe assumption.
Comment 17 Boris Zbarsky [:bz] 2008-03-17 19:13:16 PDT
Created attachment 310145 [details] [diff] [review]
Fix for both issues

Fix both issues.  Using the canonicalized spec from the URI object means that whatever lowercasing the chrome registry does we pick up.  I tested (using testcase 2) that this fixed the two uppercase tests.  The check for a scheme of "chrome" fixes the other issue.  Once I added that, the testcase just throws, of course.

The rest of the changes are just making the error reporting work with the new setup and such.  If desired, we could go back to using a naked char* for the buffer; I changed it because I was going to do other early returns in an earlier patch iteration, but I don't think that change is really needed anymore.

We should certainly do the URI thing, no matter what we decide to do about non-chrome loads here.
Comment 18 Johnny Stenback (:jst, jst@mozilla.com) 2008-03-17 20:14:32 PDT
Comment on attachment 310145 [details] [diff] [review]
Fix for both issues

Awesome.

Bob, do you have alternatives that will work for you? Do we need to add a pref or something for testing harnesses or what not?
Comment 19 Boris Zbarsky [:bz] 2008-03-17 20:17:32 PDT
I'd also appreciate help with getting tests for this into a form that can be checked in....

And waiting for bc to respond before checking this in.
Comment 20 Bob Clary [:bc:] 2008-03-17 20:30:44 PDT
I think that I can work around it by inserting a script tag into the document and then appending the script text. Go for it. I'll deal.
Comment 21 Boris Zbarsky [:bz] 2008-03-17 21:46:10 PDT
Checked in.  Would still love some help with the tests.
Comment 22 Daniel Veditz [:dveditz] 2008-03-18 12:21:10 PDT
So who's going to tell the Google Toolbar people that we took away the feature they're using?
Comment 23 Dave Townsend [:mossop] 2008-03-18 13:00:14 PDT
We should document these restrictions on MDC
Comment 24 Mark Finkle (:mfinkle) (use needinfo?) 2008-03-18 13:54:14 PDT
This could be a painful bug to take on branch. It will break extensions and we have no Components.utils.import to offer extdevs.

Currently, bz's patch only allows chrome://, but what about resource://? Components.utils.import accepts resource://

Branch has nsIResProtocolHandler, which could be used to create a bandaid for Fx2 extensions that are busted by the change.
Comment 25 Daniel Veditz [:dveditz] 2008-03-18 14:01:21 PDT
Since this change appears to break extensions we may want to wait a bit for the branch release. Definitely want a trunk beta release under our belts to see how many we break and if the workarounds for those extensions are doable on the branch.

I'd hate to have to kill all extensions by releasing a 2.0.1.x
Comment 26 Mike Shaver (:shaver -- probably not reading bugmail closely) 2008-03-18 14:06:04 PDT
If you take something that requires an active workaround on the part of extensions, you'll need to do a 2.0.1.x -- the set of broken extensions is simply not knowable (and can grow over time, if people are developing against 2.0.0.13 for a while and then ship their add-on with the reasonable 2.0.0.* maxVersion).
Comment 27 Boris Zbarsky [:bz] 2008-03-18 14:08:36 PDT
Yeah, I don't think we can, or should, take this on branch as written.

Can we get some data on what extensions actually use this functionality with non-chrome URIs?  Do these extensions actually depend on the resulting code being compiled with the system principal?  Perhaps the right fix is to not give the system principal to code not loaded from chrome://?  Frankly, using the channel principal instead of unconditionally using the system principal would make a heck of a lot more sense to me.  If we did that, we could remove the "chrome:" check.  That would break strictly less things, unless people are loading script from skin chrome packages.

Thoughts?
Comment 28 Boris Zbarsky [:bz] 2008-03-18 14:09:10 PDT
I guess someone needs to file a bug to get an AMO scan done if we actually want that sort of data...
Comment 29 Mark Finkle (:mfinkle) (use needinfo?) 2008-03-18 14:18:18 PDT
asking AMO to give us an idea on the impact by grepping addon source code (bug 423732)
Comment 30 Mark Finkle (:mfinkle) (use needinfo?) 2008-03-18 14:20:00 PDT
mozIJSSubScriptLoader.idl comments need updated
Comment 31 Boris Zbarsky [:bz] 2008-03-18 21:50:48 PDT
So I was thinking about alternate ways to do this.  Ideally, we would flag the JSScript itself instead of the filename, and then just always do wrapper automation here.  Unfortunately, the JSScript* returned from JS_CompileScript is NOT the same JSScript* as the JSFunctions that got compiled point to.  So flagging it is completely useless.  If I could pass flags down into the JS_Compile* API, that would be nice, but we'd have to get them all the way down to js_EmitTree (which is where the "real" JSScript is created).  I leave it to brendan and shaver to say what they think of that.

The other bad news is that my impression from the people bitten by this so far who've gotten a hold of me (Operator and Calendar) is that they have the following constraints:

1)  They're loading scripts from file://
2)  Their scripts really do need the system principal.

So they're precisely in the "this code may be vulnerable" corner of things and really should get wrapper automation.

Perhaps the right solution is that we should have a directory in which such scripts should be dumped and flag everything in that directory as system script?  Or something along those lines?

Alternately, we could just do what comment 9 suggests.  It'll make the JS console a little more confusing for those scripts, and venkman/firebug will have to deal, but that's life.

Reopening this and marking P1 for the time being; I don't think we can/should usefully ship the beta with the chrome check as it is.  :(  If nothing else, back out that one check before ship while we work on a better post-beta solution, I guess.  But I'd love to hear brendan and shaver's thoughts here.
Comment 32 Boris Zbarsky [:bz] 2008-03-18 21:58:31 PDT
I suppose we could also require that anyone using this put the files in chrome://, but Operator at least uses this for files the user puts somewhere....  Calendar could probably change their stuff around, I would think.

To be honest, we already do wrapper automation for some file:// URIs (component loader).  Would doing that here increase risk?

Note that we'd still want to do a protocol whitelist or something.  No one should be loading http:// URIs through here, for sure.
Comment 33 Boris Zbarsky [:bz] 2008-03-18 23:10:17 PDT
Created attachment 310413 [details] [diff] [review]
I think this might be more viable

This applies on top of the previous patch.  The change is to ensure that the URI is either chrome or nsIFileURL (so chrome, file, resource) instead of just chrome, and to go ahead and flag it as a system filename.  We could allow data: here if we wanted, but I don't see a reason to, to be honest.

This reduces concerns in comment 11 to someone we don't trust loading file://, chrome://, or resource:// JS files, and given our same-directory security protections on file:// and resource://, I think that's not something we seriously need to worry about.  But if you feel that it's a serious problem, then we should do your approach with prefixing the URI with the calling code's filename.
Comment 34 Johnny Stenback (:jst, jst@mozilla.com) 2008-03-18 23:33:32 PDT
Comment on attachment 310413 [details] [diff] [review]
I think this might be more viable

Hmm, this does scare me. Doing this for files loaded through file:// URIs from a *known* location (i.e. the profile directory or what not) seems fine, but doing this for *any* file:// URI means that if an extension loads some random js file off disk through this API, then the user loads a HTML file that includes that same js file the HTML now has access to the implicit XPCNativeWrappers and could mutate them and make our chrome vulnerable in this case.

Granted that's not an obvious nor trivial exploit to pull off, but unless we really need to, I'd rather not do this. I think I'd prefer a hack that simply fools the JS engine into flagging *this* particular load of this file as a system file, but not all loads of it.

I could probably be convinced otherwise, but those are my thoughts at the moment.

P.S. The comment in the patch above the call to FlagSystemFilenamePrefix() needs updated.
Comment 35 Boris Zbarsky [:bz] 2008-03-18 23:43:00 PDT
Calendar loads file:// URIs from the app install dir, I think.  I guess they might be able to use resource:// for that if push came to shove.

I think Operator loads from the profile.

It wouldn't be that hard, I guess, to check that the file to be loaded is in a subdir of the profile or the install dir.  That should cover all the cases we think are safe, right?

For the "this particular load" thing we'd need to set the bits on the JSScript, which means flagging the prefix, compiling/executing, then unflagging the prefix.  And having the js_NewScript code look up the bits and save them.

In any case, it sounds like we should just do what you suggested in comment 9.  I've more or less run out of time to work on this for the next few days.  :(
Comment 36 Simon Bünzli 2008-03-18 23:45:21 PDT
(In reply to comment #33)
> We could allow data: here if we wanted, but I don't see a reason to

What about dynamically loading code snippets from prefs (or other configuration files)? At least that's what I've been doing, so far (and currently don't see an alternative to).
Comment 37 Boris Zbarsky [:bz] 2008-03-18 23:50:26 PDT
It's just too easy to shoot yourself in the foot with data:, in my opinion... What would really need to be vetted is the source of the data, and there's no way to do it from this code.

If you trust the data enough, you can always write it to a file and execute that file.
Comment 38 Boris Zbarsky [:bz] 2008-03-19 10:34:44 PDT
I should also note that if you just have the JS string you can also evalInSandbox it, no?
Comment 39 Simon Bünzli 2008-03-19 11:18:45 PDT
(In reply to comment #38)
> if you just have the JS string you can also evalInSandbox it, no?

Right. Turns out that an evaluation context can even be passed to window.eval. Thanks for the hint!
Comment 40 Boris Zbarsky [:bz] 2008-03-19 20:30:46 PDT
So I'm not quite sure how to get the caller script here.  jst, if you know, do you want to write it up?  I'm not likely to be able to do the research needed to get it done before beta...

On a separate note, bug 423732 did a scan of AMO for this, and there is only one use of data:, in what looks like a unit-testing extension.
Comment 41 Johnny Stenback (:jst, jst@mozilla.com) 2008-03-20 18:28:32 PDT
Created attachment 310893 [details] [diff] [review]
Permit file:// URIs

This re-enables file:// URIs in the subscript loader, but it prepends the calling scripts filename followed by " -> " to the file:// URI so that whether the subscript gets XPCNativeWrappers or not will depend on the caller, not the file:// URI.
Comment 42 Mike Shaver (:shaver -- probably not reading bugmail closely) 2008-03-20 18:58:31 PDT
Comment on attachment 310893 [details] [diff] [review]
Permit file:// URIs

I presume this is going to require some fancy footwork in debuggers?

I'm gonna want to see some tests with that, kthx. :)
Comment 43 Boris Zbarsky [:bz] 2008-03-20 19:42:41 PDT
I think you want to use the QI to nsIFileURL that I used in my patch. Otherwise you're not allowing through resource:// URIs, which I think we do want to allow.  It should be possible to use most of my patch, minus the flagging, plus the script filename stuff.
Comment 44 Johnny Stenback (:jst, jst@mozilla.com) 2008-03-20 21:34:22 PDT
Created attachment 310920 [details] [diff] [review]
Updated fix.

This deals with file:// URIs as well as resource:// URIs, and for those prepends the caller script filename.

As we don't have any kind of system in place to write tests for this, I won't be able to do that in time for beta 5, the tests would need to place a js file in the resource directory and then make sure it's loadable, and it would need to be able to load it from both a location where implicit XPCNativeWrappers are enabled and also from where they're not enabled.
Comment 45 Boris Zbarsky [:bz] 2008-03-20 21:59:34 PDT
Comment on attachment 310920 [details] [diff] [review]
Updated fix.

Looks good.  Thank you for picking this up!
Comment 46 Johnny Stenback (:jst, jst@mozilla.com) 2008-03-21 00:08:08 PDT
Fix checked in.
Comment 47 Mike Shaver (:shaver -- probably not reading bugmail closely) 2008-03-21 10:05:41 PDT
(In reply to comment #44)
> Created an attachment (id=310920) [details]
> Updated fix.
> 
> This deals with file:// URIs as well as resource:// URIs, and for those
> prepends the caller script filename.
> 
> As we don't have any kind of system in place to write tests for this, I won't
> be able to do that in time for beta 5

I'm surprised you can't test subscript loader's resulting privs with xpcshell tests, at least partially?

(Can you at least file a bug describing what sort of system you'd need to write tests for it, so it's not just forgotten after b5?)

> the tests would need to place a js file
> in the resource directory and then make sure it's loadable, and it would need
> to be able to load it from both a location where implicit XPCNativeWrappers are
> enabled and also from where they're not enabled.

I don't quite understand this part -- won't those tests pass on a build without this patch as well?  Doesn't seem like they test that this bug is (still) fixed.
Comment 48 Boris Zbarsky [:bz] 2008-03-21 20:55:46 PDT
We should really test the following things for this bug:

1) resource:// URIs are loadable
2) file:// URIs are loadable
3) chrome:// URIs are loadable
4) Other URIs are not loadable.  Especially, http:// URIs are not loadable.
5) Loading a loadable chrome URI does XPCNativeWrapper automation based on that
   chrome URI.
6) Loading a file:// or resource:// URI does XPCNativeWrapper automation based
   on the URI of the caller of loadSubscript.
7) Make sure to test various cases for chrome:// URIs.
8) Make sure that in all cases the loaded script is executed with the system
   principal.

We should be able to test (4) with a mochitest.  To test the rest, we need to place files that could be loadable at chrome://, file:// and resource:// URIs.  file:// is doable with a mochitest: just create a file in /tmp as part of the test, write data to it, and try to loadSubscript() it, then remove it when done.  It _might_ be possible to do the same with resource:// by using the app dir instead of /tmp?  I'm not sure how to do chrome:// offhand.  Ideally, we would have a basic system for placing things at such URIs, either via makefile (likely needed for chrome) or via some SimpleTest methods (just pass the string you want in the file, and it'll create the file for you, etc).

So I think we can test 1, 2, 4, 8 (except the chrome:// case for 8) as things stand.  To test 3, 5, 6, we need to figure out a way to run a test as part of a chrome package where we control wrapper automation and need a way to be loading a chrome:// file.
Comment 49 Boris Zbarsky [:bz] 2008-03-21 21:12:03 PDT
Filed bug 424483 and bug 424484 for some of the infrastructure improvements that could be useful here.
Comment 50 Boris Zbarsky [:bz] 2008-03-21 21:20:31 PDT
I spun off bug 424485 on writing the tests for this.
Comment 51 Johnny Stenback (:jst, jst@mozilla.com) 2008-06-02 14:54:56 PDT
This broke at least the Google Notebook extension when it went in for Firefox 3, see bug 427902. An updated version of the Google Notebook extension exists now, but I don't know if that one works with Firefox 2, or what other extensions would break if we took this on the branch. If someone could test at least the Google Notebook extension, and that still works, I could port this to the branch. Shouldn't be too much work...
Comment 52 Samuel Sidler (old account; do not CC) 2008-06-02 21:27:49 PDT
Al, can you confirm that the latest Google Notebook extension works on branch and isn't only trunk-compatible?
Comment 53 Al Billings [:abillings] 2008-06-03 10:30:58 PDT
It installs and worked in the latest branch nightly (Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.15pre) Gecko/2008060303 BonEcho/2.0.0.15pre).
Comment 54 Johnny Stenback (:jst, jst@mozilla.com) 2008-06-04 12:58:07 PDT
Created attachment 323747 [details] [diff] [review]
1.8 branch version.

This is the same as the trunk version except for the change to NS_OpenURI() in
nsNetUtils.h which was needed by this fix. (attaching to the right bug this time)
Comment 55 Boris Zbarsky [:bz] 2008-06-04 14:06:51 PDT
Comment on attachment 323747 [details] [diff] [review]
1.8 branch version.

Looks good.
Comment 56 Daniel Veditz [:dveditz] 2008-06-05 14:17:41 PDT
Comment on attachment 323747 [details] [diff] [review]
1.8 branch version.

Approved for 1.8.1.15, a=dveditz for release-drivers
Comment 57 Johnny Stenback (:jst, jst@mozilla.com) 2008-06-05 14:37:41 PDT
Fixed on the branch.

QA: It is essential for this to get extra extension compatibility testing before 1.8.1.15 goes out the door!
Comment 58 Mike Kaply [:mkaply] 2008-06-05 16:17:24 PDT
Especially the JavaScript Debugger
Comment 59 Al Billings [:abillings] 2008-06-05 17:01:39 PDT
Do you guys have a list so QA isn't simply guessing?

The Google Notebook extension is obvious. 

Michael, which JavaScript Debugger? Perhaps it is obvious to others but not to me.
Comment 60 Mark Finkle (:mfinkle) (use needinfo?) 2008-06-05 19:07:47 PDT
(In reply to comment #59)
> Do you guys have a list so QA isn't simply guessing?

Al, see http://starkravingfinkle.org/blog/2008/03/extension-developers-breaking-news-part-2/ comments for a list of extensions that needed to be fixed in FF3. I assume these will be affected in FF2 as well.
Comment 61 :Gijs Kruitbosch (away 26-29 incl.) 2008-06-05 23:28:30 PDT
(In reply to comment #59)
> Do you guys have a list so QA isn't simply guessing?
> 
> The Google Notebook extension is obvious. 
> 
> Michael, which JavaScript Debugger? Perhaps it is obvious to others but not to
> me.
> 

Venkman as well as Firebug, I'd presume, though I would guess that mkaply was referring to the former.
Comment 62 Al Billings [:abillings] 2008-06-10 17:17:15 PDT
Testcase 1 doesn't seem to do anything in either 2.0.0.14 or 2.0.0.15 (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.15pre) Gecko/2008061005 BonEcho/2.0.0.15pre). Testcase 2 is verified to no longer work in 2.0.0.15.

The Google Notebook extension works without any issues.
I verified Firebug and Venkman as well.

In nightly 1.8 Thunderbird (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.15pre) Gecko/2008061003 Lightning/0.8 Thunderbird/2.0.0.15pre), I installed Lightning .8, since there were concerns about this completely breaking lightning as prevoiously written. Lightning ran without any noticeable errors (I imported an .ics calendar file, got alerts, looked at events).

I also installed Chatzilla and verified that plugins for it are working (this was a problem previously reported). 

I asked Ctalbert, who has been working with MozRepl, to check to see if that still works. I asked StephenD, who has been working with Selenium, to check those as well.

Comment 63 Stephen Donner [:stephend] 2008-06-10 17:54:56 PDT
I recorded and ran a Selenium IDE script against http://addons.mozilla.org using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.15pre) Gecko/2008061005 BonEcho/2.0.0.15pre just fine.
Comment 64 cmtalbert 2008-06-12 11:10:05 PDT
Ran a simple automation test using MozRepl (http://hyperstruct.net/projects/mozlab) and it worked great on Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.8.1.15pre) Gecko/2008061104 BonEcho/2.0.0.15pre
Comment 65 Al Billings [:abillings] 2008-06-12 11:37:28 PDT
I'm marking this as verified for 1.8.1.15 then.
Comment 66 Daniel Boelzle [:dbo] 2008-08-07 07:45:46 PDT
I am having (relatively new) issues with venkman loading scripts outside the components/ directory in lightning; some are hosted in js/. The scripts are properly loaded and listed (lightning works etc), but clicking them in venkman raises errors like:

Error loading URL <file:/Users/dbo/profiles/thunderbird/mst/extensions/%7Be2fda1a4-762b-4020-b5ad-a41df1933103%7D/components/calItemModule.js -> file:///Users/dbo/profiles/thunderbird/mst/extensions/%7Be2fda1a4-762b-4020-b5ad-a41df1933103%7D/js/calAttendee.js>: [Exception... "Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIChannel.asyncOpen]" nsresult: "0x80520012 (NS_ERROR_FILE_NOT_FOUND)" location: "JS frame :: chrome://venkman/content/venkman-url-loader.js :: loadURLAsync :: line 79" data: no].

I am not sure whether this relates to this bug/change (and my updated thunderbird 2), though, but I can reproduce it with lightning 0.8, thus can exclude a recent change in lightning has caused it.
Comment 67 James Ross 2008-08-07 08:11:12 PDT
I'm pretty sure it is caused by this bug's change, as Venkman doesn't know what to make of the "URI1 -> URI2" syntax (file a bug if there isn't one, please, so Venkman can be fixed to load the correct URI). In the mean time, you should be able to use Pretty Print to debug these scripts.
Comment 68 Alexander Sack 2009-01-05 11:44:35 PST
Comment on attachment 323747 [details] [diff] [review]
1.8 branch version.

a=asac for 1.8.0
Comment 69 Eric Shepherd [:sheppy] 2009-02-16 17:44:06 PST
Documentation updated.

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