1268396 - Write testcase for nsIDocShell::APP_TYPE_EDITOR

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Christoph Kerschbaumer [:ckerschb]]

As discussed within [1] we should have a testcase for nsIDocShell::APP_TYPE_EDITOR. Boris suggests we could do the following:

1)  Opens a tab.
2)  Grabs hold of the nsIDocShell for that tab; with the e10s fun involved.
3)  Sets .appType on that nsIDocShell to nsIDocShell::APP_TYPE_EDITOR.
4)  Loads some unprivileged (e.g. http://) URI in that tab that contains an <img> pointing
    to some privileged (file:// or non-exposed chrome:// or something) URI.
5)  Checks that the image load succeeds.

Steps 2 and 4 are where the non-simplicity is, of course.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1206961#c83

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1268396_test_for_app_type_editor.patch

Boris, PRIV_IMG is not accessible to content, hence we could use it for testing that a docshell of appType TYPE_EDITOR can access the URI from an unprivileged http:// URL.

I know that Gijs would prefer if we register our own image for the test instead of relying on an image from devtools code. I added a comment on top of PRIV_IMG so that we know we have to find a different privileged image in case we ever remove that img.

I would rather not go through the trouble of registering and unregistering an image for that test. If you insist, I would obviously find a way to make that work, but I am not sure it's worth the trouble.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8746647 [details] [diff] [review]
bug_1268396_test_for_app_type_editor.patch

I think I'm OK with not registering an image specially for this, though it might be good to just have one on hand for whatever other tests we have for this stuff (or should have!).

>+      let docShell = browser.contentWindow.QueryInterface(Ci.nsIInterfaceRequestor)

Does this work in e10s?

r=me modulo that.

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1268396_test_for_app_type_editor.patch

Blake, can you please take a quick look at that test and let me know what I might be doing wrong? In short, the test modifies the appType of the docshell so certain security checks are bypassed within nsContentUtils::CanLoadImage().

The test works fine using e10s but when running:
> ./mach mochitest --disable-e10s ...
it seems the modified appType on the docshell gets lost along the way and is back to APP_TYPE_UNKNOWN here [1].

I am using  ContentTask.spawn() for the first time. I am doing something wrong?

[1] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsContentUtils.cpp#3010

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8747044 [details] [diff] [review]
bug_1268396_test_for_app_type_editor.patch

Review of attachment 8747044 [details] [diff] [review]:
-----------------------------------------------------------------

I ended up looking at this in a debugger. The key is [1]. In other words, that function looks at the *root* docshell for the loading image. In e10s, the root docshell is the content docshell. In non-e10s there's a chrome docshell serving as the root. Therefore, when you set the itemType of the content docshell in non-e10s, we end up ignoring it and using the root docshell's type (which is a normal docshell).

[1] https://dxr.mozilla.org/mozilla-central/rev/77cead2cd20300623eea2416bc9bce4d5021df09/dom/base/nsContentUtils.cpp#2998-3001

::: image/test/browser/browser_docshell_type_editor.js
@@ +18,5 @@
> +  }, function* (browser) {
> +    yield ContentTask.spawn(browser, null, function* () {
> +      let docShell = content.QueryInterface(Ci.nsIInterfaceRequestor)
> +                            .getInterface(Ci.nsIWebNavigation)
> +                            .QueryInterface(Ci.nsIDocShell);

Nit: content tasks have a |docShell| property immediately available without having to go through |content|.

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Blake Kaplan (:mrbkap) (please use needinfo!) from comment #4)
> I ended up looking at this in a debugger. The key is [1]. In other words,
> that function looks at the *root* docshell for the loading image. In e10s,
> the root docshell is the content docshell. In non-e10s there's a chrome
> docshell serving as the root. Therefore, when you set the itemType of the
> content docshell in non-e10s, we end up ignoring it and using the root
> docshell's type (which is a normal docshell).

Thanks Blake for the detailed analysis. I should have just used the debugger myself, but also wanted to make sure there is no weird behavior going on - but that makes perfect sense. Test works just fine now!

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1268396_test_for_app_type_editor.patch

Using the rootDocShell instead of the current docShell; carrying over r+ from bz.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9b7ebd2e2e42

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9b7ebd2e2e42

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8748096 [details] [diff] [review]
bug_1268396_test_for_app_type_editor.patch

Er, wait.  If you mess with the _root_ docshell in non-e10s, you really need to restore it back to the original state: that's the entire browser window.  In particular, all CheckLoadURI tests for images that run after this test would no longer work correctly!

Alternately, make the test open a new window to do its stuff in or something, so you can later close it and not worry about the fact that you've put it in a weird state.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Please do address comment 9.  Right now we have a time-bomb depending on what tests run after this one in the same chunk.

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Boris Zbarsky [:bz] from comment #10)
> Please do address comment 9.  Right now we have a time-bomb depending on
> what tests run after this one in the same chunk.

Oh, I thought at least the two add_task() run in sequence, or is that a wrong assumption? If they run in sequence we are good, see [1]. If I am wrong I will fix that right away of course.

[1] http://mxr.mozilla.org/mozilla-central/source/image/test/browser/browser_docshell_type_editor.js#61

Comment 12

[Boris Zbarsky [:bzbarsky]]

Hmm.  I don't know what guarantees there are for the two add_task, but I do think they run in order, and it's probably OK in the sense that by end of this test we're in a sane state.  Please do document the ordering dependency here, though!

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1268396_followup.patch

Boris, you are right, to be on the safe side we should restore the default appType of the rootDocShell before moving on to the next test.

Thanks for following up on this patch!

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8748257 [details] [diff] [review]
bug_1268396_followup.patch

>+          //restore appType of rootDocShell before moving on to the next test

Space after "//" please, all four places.  r=me.  Thank you!

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/af54ea165cfd

Comment 16

[Carsten Book [:Tomcat]]

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