771354 - Loading remote xul crashes gecko

Status: RESOLVED FIXED

(Core :: Security: CAPS, defect)

Description

[Malini Das [:mdas] - Away, not checking bugmail]

Attachment: mochitest chrome test

We load some remote xul for Marionette unit testing purposes by setting the profile's "dom.allow_XUL_XBL_for_file" preference to true, and loading some locally hosted xul. 

The .xul file used is attached (test.xul). It hold a dialog with some elements in it. Sometime after last week we've been seeing this crash:

WARNING: Not same origin error!: file /Users/mdas/code/mozilla-central/dom/base/nsJSEnvironment.cpp, line 350
JavaScript error: chrome://global/content/bindings/dialog.xml, line 196: Permission denied for <file://> to get property XPCComponents.classes


To reproduce this error, copy test.xul somewhere to your file system. Copy  test_file_fail.xul into your (objdir)/_tests/testing/mochitest/chrome directory and replace the YOUR_PATH_HERE text in test_file_fail.xul with the file path to test.xul (for example: file:///Users/mdas/test.xul). Run it with:

python runtests.py --chrome --test-path=test_file_fail.xul --setpref="dom.allow_XUL_XBL_for_file=true"

Once you click and run the test, the gecko process will soon crash, and you will see the Permission Denied message above.

From our tests, it seems the change that triggered this happened sometime between these commits: https://github.com/mozilla/mozilla-central/compare/6b7e00d441baa574c0c62f3cc9ef01482223009a...2be4e380b4fb6ffc71a98c96de2d30cd48eeb8f2

Comment 1

[Malini Das [:mdas] - Away, not checking bugmail]

Attachment: 'remote' xul to be loaded

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Malini, can you run it under gdb and provide a crash stack? Also will you be able to get a smaller regression range? Is this code on github a 1-1 clone from hg.mozilla.org/mozilla-central?

Comment 3

[Bobby Holley (:bholley)]

This sounds more like a regression from bug 754202, but I don't see that in the revision range.

Comment 4

[Malini Das [:mdas] - Away, not checking bugmail]

Attachment: backtrace

Here's the backtrace after the crash

Comment 5

[Malini Das [:mdas] - Away, not checking bugmail]

Attachment: backtrace for all threads

output of `thread apply all bt full`

Comment 6

[Benjamin Smedberg]

What revision is this stacktrace against, so we can match up line numbers?

Comment 7

[Malini Das [:mdas] - Away, not checking bugmail]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #6)
> What revision is this stacktrace against, so we can match up line numbers?

https://hg.mozilla.org/mozilla-central/rev/47f814827db6

Comment 8

[Malini Das [:mdas] - Away, not checking bugmail]

(In reply to Henrik Skupin (:whimboo) from comment #2)
> Malini, can you run it under gdb and provide a crash stack? Also will you be
> able to get a smaller regression range? Is this code on github a 1-1 clone
> from hg.mozilla.org/mozilla-central?

I'll try to get a better regression range, and yes, this code is a 1-1 clone.

Comment 9

[Benjamin Smedberg]

Crash is at http://hg.mozilla.org/mozilla-central/annotate/47f814827db6/dom/base/nsJSEnvironment.cpp#l1222

1221 principal->Equals(scopeObjectPrincipal, &equal);
1222 MOZ_ASSERT(equal);

Comment 10

[Jonathan Griffin (:jgriffin)]

(In reply to Bobby Holley (:bholley) from comment #3)
> This sounds more like a regression from bug 754202, but I don't see that in
> the revision range.

Yep, that was my fault, that regression range was for another crashing bug, not this one.  The m-c commit this first appears on is http://hg.mozilla.org/mozilla-central/rev/4a8e0d5fc954.

Comment 11

[Jonathan Griffin (:jgriffin)]

And tracing that back through the mozilla-inbound merge, this problem first appears at http://hg.mozilla.org/integration/mozilla-inbound/rev/cf09f7b139da.

Comment 12

[Bobby Holley (:bholley)]

I'm totally willing to believe that this is happening, but I can't reproduce it with the STR in comment 0. I see the two warnings, but I don't get a crash.

Comment 13

[Bobby Holley (:bholley)]

From the looks of it, we're getting a mismatch in principals between the XUL element and the binding. I'm guessing we should just pass the principal from the bound content to the field installer.

Comment 14

[Malini Das [:mdas] - Away, not checking bugmail]

(In reply to Bobby Holley (:bholley) from comment #12)
> I'm totally willing to believe that this is happening, but I can't reproduce
> it with the STR in comment 0. I see the two warnings, but I don't get a
> crash.

That's odd, I just pulled from m-c today and I can reproduce the error. Can you verify that you passed the `--setpref="dom.allow_XUL_XBL_for_file=true"` parameter to the runtests.py program? If you don't set that pref, then it'll ignore the file and you just get warnings.

Comment 15

[Bobby Holley (:bholley)]

I see the XUL popup window (box, test,test,test, etc), so presumably that part's working. Maybe it's a platform thing? I'm on mac.

Anyway, I understand the problem and it should be straightforward to fix, but I need a way to reproduce it first. Can you try to put together a reduced crashtest? Or is there no way to do remote xul under the reftest (or mochitest) harness?

Comment 16

[Malini Das [:mdas] - Away, not checking bugmail]

(In reply to Bobby Holley (:bholley) from comment #15)
> I see the XUL popup window (box, test,test,test, etc), so presumably that
> part's working. Maybe it's a platform thing? I'm on mac.
> 
> Anyway, I understand the problem and it should be straightforward to fix,
> but I need a way to reproduce it first. Can you try to put together a
> reduced crashtest? Or is there no way to do remote xul under the reftest (or
> mochitest) harness?

I'm not too familiar with the harness, I can see if there's another way though, unless someone here knows of a way.

I'm also running on a mac, 10.6.8

Comment 17

[Bobby Holley (:bholley)]

Sorry I disappeared last night (was dinner time over here in Europe). Were you able to reproduce the crash with a clean build?

Comment 18

[Jonathan Griffin (:jgriffin)]

I reproduced with the latest m-c build:  http://stage.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux-debug/1341933030/firefox-16.0a1.en-US.linux-i686.tar.bz2

To reproduce:  download build, launch it, and add the following prefs:

marionette.defaultPrefs.enabled = true
dom.allow_XUL_XBL_for_file = true

Then restart it.

Next, go to m-c/testing/marionette/client/marionette, and execute the following command:

python runtests.py --address localhost:2828 tests/unit/test_text.py

You may need to install some python packages first, but these are available via pypi:  datazilla, manifestparser, mozhttpd, mozprocess

Comment 19

[Malini Das [:mdas] - Away, not checking bugmail]

Oh, you'll need python2.7 to run the test in Comment 18

If for whatever reason you don't want to download those packages into your python path, we have a virtualenv you can use. You can go to m-c/testing/marionette/client/marionette and then type:

sh venv_test.sh <path to python2.7>

This will create a marionette_venv directory in m-c/testing/marionette/client, and if you cd into that directory and call

. bin/activate

you'll have a python virtual environment with all the packages pre-installed. You can then go back to m-c/testing/marionette/client/marionette and run:

python runtests.py --address localhost:2828 tests/unit/test_text.py

To get out of the virtual environment, just type 'deactivate'

Comment 20

[Malini Das [:mdas] - Away, not checking bugmail]

Hm once bug 771224 lands, we won't be able to reproduce this with the latest builds, you'd have to run them from the previous debug builds (ftp://ftp.mozilla.org/pub/firefox/tinderbox-builds/mozilla-central-macosx64-debug/ for osx 64bit). 

I'm not sure what to do as a test alternative, since the mochitest failed to reproduce on bholley's machine.

Comment 21

[Bobby Holley (:bholley)]

Ok, I managed to reproduce this by rewinding to 5b77d71ed927, building with ENABLE_MARIONETTE=1, enabling the virtual environment, and running the test. I'll take a crack at fixing the bug next week. Thanks for all the hard work helping me reproduce! ;-)

Comment 24

[Bobby Holley (:bholley)]

Attachment: Don't special-case principal assignment for chrome windows in nsGlobalWindow.cpp. v1

This is a simple patch that applies on top of bug 774607, whose correctness in turn depends on bug 774633.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 646210 [details] [diff] [review]
Don't special-case principal assignment for chrome windows in nsGlobalWindow.cpp. v1

r=me

Comment 26

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/670c8a2695b3

Comment 27

[Matt Brubeck (:mbrubeck)]

Backed out along with other patches from the same push, because of possibly causing timeouts in 'make check' on Windows:
https://hg.mozilla.org/integration/mozilla-inbound/rev/deeadcce3f64
https://tbpl.mozilla.org/php/getParsedLog.php?id=14652164&tree=Mozilla-Inbound

Comment 28

[Bobby Holley (:bholley)]

Relanded to m-i with the fix for the hanging windows make check:

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=cfc884e6e414

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cfc884e6e414

Comment 30

[Bobby Holley (:bholley)]

Pushed to m-a:
https://hg.mozilla.org/releases/mozilla-aurora/pushloghtml?changeset=b8ed43695721

Comment 31

[Bobby Holley (:bholley)]

I didn't test this stuff with the marionette environment recently, and I seem to have lost track of it. If someone wants to make verify this, use the steps in comment 18.

Comment 32

[Bobby Holley (:bholley)]

Pushed to m-b:
https://hg.mozilla.org/releases/mozilla-beta/pushloghtml?changeset=6fbaf3aed7a0

Note that the approval here comes from bug 774633. The patches are inextricably tied, and it's just a historical artifact that they're split across separate bugs.

Comment 33

[Ioana (away)]

I have been trying to verify this bug, but I ran into a problem. If anyone here can help me, please do so. Here are the details:

When I try to verify this fix with the steps from comment 18, I get this http://pastebin.mozilla.org/1861886.

I built the client locally, with Marionette enabled. I also set the following preferences, and restarted the client, before trying to run the test:
marionette.defaultPrefs.enabled;true
marionette.defaultPrefs.port;2828
dom.allow_XUL_XBL_for_file;true

I used the mozilla-central repo for the build, since I was told that Marionette is broken on beta.

Comment 34

[Jonathan Griffin (:jgriffin)]

(In reply to Ioana Budnar [QA] from comment #33)
> I have been trying to verify this bug, but I ran into a problem. If anyone
> here can help me, please do so. Here are the details:
> 
> When I try to verify this fix with the steps from comment 18, I get this
> http://pastebin.mozilla.org/1861886.
> 
> I built the client locally, with Marionette enabled. I also set the
> following preferences, and restarted the client, before trying to run the
> test:
> marionette.defaultPrefs.enabled;true
> marionette.defaultPrefs.port;2828
> dom.allow_XUL_XBL_for_file;true
> 
> I used the mozilla-central repo for the build, since I was told that
> Marionette is broken on beta.

That should do the trick. How did you enable Marionette in your build?  In case you didn't know, you need to add

ENABLE_MARIONETTE=1

in your mozconfig before building.

Comment 35

[Malini Das [:mdas] - Away, not checking bugmail]

(In reply to Jonathan Griffin (:jgriffin) from comment #34)
> (In reply to Ioana Budnar [QA] from comment #33)
> > I have been trying (In reply to Ioana Budnar [QA] from comment #33)
> I have been trying to verify this bug, but I ran into a problem. If anyone
> here can help me, please do so. Here are the details:
> 
> When I try to verify this fix with the steps from comment 18, I get this
> http://pastebin.mozilla.org/1861886.
> 
> I built the client locally, with Marionette enabled. I also set the
> following preferences, and restarted the client, before trying to run the
> test:
> marionette.defaultPrefs.enabled;true
> marionette.defaultPrefs.port;2828
> dom.allow_XUL_XBL_for_file;true
> 
> I used the mozilla-central repo for the build, since I was told that
> Marionette is broken on beta.

I just pulled mozilla-central today and built, and had no problems running tests. The following is what my mozconfig has.

mk_add_options MOZ_OBJDIR=@TOPSRCDIR@/../obj-ff
mk_add_options AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213
. $topsrcdir/browser/config/mozconfig
mk_add_options MOZ_MAKE_FLAGS="-j4"
export CC=/usr/local/bin//clang
export CXX=/usr/local/bin//clang++

ac_add_options --enable-debug
ac_add_options --disable-optimize

ENABLE_MARIONETTE=1

The only thing of importance here is "ENABLE_MARIONETTE=1"

As for the profile, my user.js file only contains:
pref("marionette.defaultPrefs.enabled", true);

Hope this helps!

Comment 36

[Ioana (away)]

Jonathan, Malini, I had this in my mozconfig:
ENABLE_MARIONETTE=1
ac_add_options --enable-debug

Malini, are any of the other options in your mozconfig related to Marionette?

Comment 37

[Jonathan Griffin (:jgriffin)]

(In reply to Ioana Budnar [QA] from comment #36)
> Jonathan, Malini, I had this in my mozconfig:
> ENABLE_MARIONETTE=1
> ac_add_options --enable-debug
> 
> Malini, are any of the other options in your mozconfig related to Marionette?

That should do the trick.

Comment 38

[Ioana (away)]

(In reply to Jonathan Griffin (:jgriffin) from comment #37)
> (In reply to Ioana Budnar [QA] from comment #36)
> > Jonathan, Malini, I had this in my mozconfig:
> > ENABLE_MARIONETTE=1
> > ac_add_options --enable-debug
> > 
> > Malini, are any of the other options in your mozconfig related to Marionette?
> 
> That should do the trick.

It's not doing the trick for me yet. Any ideas on what the problem could be?