445004 - [FIX]Plugins embedded in generated file:// loaded pages don't appear and don't play.

Status: RESOLVED FIXED

(Core :: Security, defect, P2)

Description

[Tom Nowacki]

User-Agent:       Mozilla/4.0 (compatible; MSIE 7.0; Windows NT 5.1; .NET CLR 1.1.4322)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.9) Gecko/2008052906 Firefox/3.0

When generating frame content, a media source does not open in the generated frame. I tested this with QuickTime (*.mov media) and WMP (*.rmi media). I believe the problem affects all media embedded locally.

Reproducible: Always

Steps to Reproduce:
1. Open local example file: midifram.htm
2. In top frame click: Generate
3. The lower frame generates, but plugin isn't showing up
Actual Results:  
Html code loads, but the plugin appears not to be loaded.

Expected Results:  
The plugin should play the midi file.

For me it becomes serious with offline pages as I currently have them, so I'll label the severity as major.

Comment 1

[Tom Nowacki]

Attachment: extract files and open midifram.htm

I wondered if this was a local security restriction and you could somehow turn off the setting, but I think it's bug.

Comment 2

[Tom Nowacki]

Attachment: extract files and open new.htm

I just noticed (testing it again) that the behavior isn't always reproduced. Sometimes the plugin plays. Sometimes it doesn't. Why?

Here is an earlier example I had which always breaks the plugin embedding. You open both frames in a new window first. In the new window the generated plugin never plays.

Steps:
1. Open file new.htm in folder root
2. In top frame click: Generate
3. plugin isn't loaded

Comment 3

[Ria Klaassen (not reading all bugmail)]

I only hear music in Opera 9.5.

Comment 4

[Tom Nowacki]

The object tag is mozilla compatible, in this case, and you require the Windows Media Player Firefox Plugin (npnul32.dll), or the older version (npdsplay.dll) to play pre1bach.mid. I omitted the internet explorer alternate object tag from the generated code, because it isn't needed here. 

You should not just hear music, you should see the plugin on the page. But neither works. I've also tested with QuickTime and it does not embed the plugin using the attachment (id=329316) example.

http:// protocol or FF2 works without a hitch, as it should.

Older Opera is not very good with plugins from what I hear, newer Opera I think it is compatible with the WMP plugin.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

FYI, npnul32.dll isn't at all related to WMP...

Comment 6

[Tom Nowacki]

np-mswmp.dll Oops.

Comment 7

[Tom Nowacki]

The issue seems to be related to file origin. Perhaps if the media file is in another folder the plugin can't open? 

I know in beta releases there were problems with local scripting across frames in different folders. Those could be resolved by setting

pref("security.fileuri.strict_origin_policy", true);

in grepref/all.js to false, reverting to the traditional same-origin policy mode. That doesn't fix this problem.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Hmm... I don't have a Windows setup yet (working on it).  If someone who does can reproduce, can you find a regression range?

Comment 9

[Tom Nowacki]

I can only narrow it down to after Gecko/2008012604 Minefield/3.0b3pre and
before Gecko/2008032620 Firefox/3.0b5 (so far - working on it too.) Where I can download builds in between?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Tom, thanks for working on this!  You should be able to get nightly builds in that range at <http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2008/>.  Then pick the month (01, 02, 03 in this case), then a day.

Comment 11

[Tom Nowacki]

It's the least I could do.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b5pre) Gecko/2008032106 Minefield/3.0b5pre

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b5pre) Gecko/2008032204 Minefield/3.0b5pre

(and before) test negative.

---------------------------
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b5pre) Gecko/2008032304 Minefield/3.0b5pre

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b5pre) Gecko/2008032405 Minefield/3.0b5pre

(and later) test positive.

---------------------------

Comment 12

[Boris Zbarsky [:bzbarsky]]

Arg.  This is in fact a regression from bug 402983, and sorta my fault.  In bug 402983 comment 10 I said:

  Could we perhaps do something where a subframe that has a file:// principal
  also stores the principal of its parent frame, or better yet of whoever
  loaded it.  If the latter is also a file:// principal, it's used for
  same-origin checks.

  In other words, the principals for a frame and its parent test equal, but
  still have different URIs (so that base URI stuff works right).

but what we ended up implementing (and I should have caught this in review) just uses the parent frame principal.  Which means that base URIs do NOT work right anymore, as in this case.  In other words, since we set the base URI of the JS-generated frame based on the principal of the JS, and since we now inherit file principals, we end up with the wrong base URI (that of the page that opened the frameset, not of the frame in question).

We really need to fix this.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Looks like bug 442244 also talks about this issue.  Setting dependency.

Comment 14

[Samuel Sidler (old account; do not CC)]

Not blocking since there's no patch and such. Boris, should you own this?

Comment 15

[Johnny Stenback (:jst)]

Boris, marking this as a P3 blocker for you, meaning it doesn't need a beta to bake in.

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Why are base URIs affected by principal? They should be orthogonal, no?

Comment 17

[Boris Zbarsky [:bzbarsky]]

Because we're generating a page via document.write, so we set its base URI to the principal URI of the caller.

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Couldn't we set it to the baseURI of the caller? That seems more correct.

Comment 19

[Boris Zbarsky [:bzbarsky]]

We don't have that.  We just have the principal, gotten off the JS stack.

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

However we fix this it sounds scary enough that it needs beta baking, so setting to P2.

Couldn't we grab the calling context instead of the calling principal? And get the document out of that?

Comment 21

[Boris Zbarsky [:bzbarsky]]

Fundamentally, we use the principal URI as base URI in various places, not just document.write.  So we just need to make sure it's correct...

Comment 22

[Boris Zbarsky [:bzbarsky]]

I guess we can't change nsIPrincipal on 1.9.0, right?  So I need to work around that somehow?

Comment 23

[Boris Zbarsky [:bzbarsky]]

> Couldn't we grab the calling context instead of the calling principal?

Oh, and as far as I undertand, no.  If code in window A calls code in window B that does a document.write in window C, then as far as I can see all the JS is running on the JSContext of window A, and that's all we can get as far as contexts go...  Maybe I'm wrong, of course.

Comment 24

[Boris Zbarsky [:bzbarsky]]

OK, so I was wrong in comment 21.  The only place where we use the principal for the base URI is right here, in OpenCommon.

So then I did a bit of digging into the behavior here in other browsers and the history of our behavior.  The basic testcase is two windows, A and B, with different base URIs.  Window B has functions that can either set location or document.write into subframes of window B.  It either calls those itself, or A calls them.  I'll roll such a test into the mochitests I write for this bug.

IE 7's behavior seems to be the following in a simple case with two static documents (so no document.write or javascript: magic):
B sets location.href directly: document URI of B is used to resolve relative URI
B calls document.write directly: target gets B's document URI as URI and base
                                 URI
A sets location.href via B: document URI of A is used to resolve relative URI
A calls document.write via B: target gets document URI of A as URI and base URI

Opera 9.6 on Mac behavior is slightly different (saner, too):
B sets location.href directly: base URI of B is used to resolve relative URI
B calls document.write directly: target gets B's document URI as URI and B's
                                 base URI as base URI.
A sets location.href via B: base URI of A is used to resolve relative URI
A calls document.write via B: target gets document URI of A as URI and base URI
                              of A as base URI

Safari 3.1.2 and a Webkit nightly match Opera except in the "A calls document.write via B" case, in which case the base URI and document URI of B are used, not those of A.

Gecko matches Safari for document URIs as long as the principal URI for a document's NodePrincipal() is the document URI.  In Gecko 1.8 this is the case for file:// while in Gecko 1.9 it's not.  In Gecko 1.9 for file://, if A loaded B we end up using the document URI of A for everything.  In Gecko, the base URI of a document.open-created document is always equal to its document URI.

Revision history looks something like this.
1) We used to use GetDocumentFromCaller() and then use that document's
   documentURI for the URI of the newly-opened document.  This broke cases
   where the caller was a javascript: document, since we don't give those the
   URI of whatever created them.  In those cases the newly-opened document
   ended up not being same-origin with the caller, which was bad.
2) In bug 57600 we started giving the newly-opened document its caller
   document's principal and using the URI from the principal (or about:blank
   for system principals).
3) This caused security issues, because GetDocumentFromCaller() didn't really
   work that well.  Or more precisely it didn't give us the principal we want.
   So in bug 296830 we switched to using GetSubjectPrincipal() to get the
   principal.  We continued to use GetDocumentFromCaller() to get the
   securityInfo, though.
4) We fixed this in bug 298064 by switching GetDocumentFromCaller() to get the
   document via the JS callstack instead of just getting it via the 
   JSContext/nsIScriptContext.
5) This broke the securityInfo handling in some cases, so in bug 312363 we
   effectively backed out bug 298064 as far as document.open is concerned (at
   least if I read this correctly; blake, can you confirm?).  We needed the
   fixed GetDocumentFromCaller() in other places, so we switched to
   GetDocumentFromContext() (which did what GetDocumentFromCaller() used to do
   before bug 298064).

A lot of this would have been simpler if we'd simply had tests people actually ran as we did all this.  :(

Comment 25

[Boris Zbarsky [:bzbarsky]]

I think we pretty much want Opera's behavior here, in general.  It matches IE's when there are no base URIs around, and is much saner when there are.

One way to do that would be to get the URI and base URI for the document.opened document from the document we get the security info from (the one returned by GetDocumentFromContext()).  We presumably want to keep using the subject principal for the principal, so as not to regress bug 296830, though I'd like to double-check that.

Unfortunately, this would almost certainly cause issues for documents created via javascript: which then call document.write(), because I'm pretty sure that we don't give those the same document URI or base URI that other browsers do.  See bug 49312 and its dependencies, and especially bug 210862 comment 15.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Oh, for additional fun, we do weird stuff with location sets from documents loaded using javascript: (said weird stuff being added in bug 162128).  Perhaps we should simply do the same for document.write as a quick fix if I can't think of a better way to really fix this.

Comment 27

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #24)
> 5) This broke the securityInfo handling in some cases, so in bug 312363 we
>    effectively backed out bug 298064 as far as document.open is concerned (at
>    least if I read this correctly; blake, can you confirm?).  We needed the
>    fixed GetDocumentFromCaller() in other places, so we switched to
>    GetDocumentFromContext() (which did what GetDocumentFromCaller() used to do
>    before bug 298064).

Yeah, that's what happened there.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix

There are a few things going on here.  I decided to not mess with the principals after all, and just fix document.write, based on comment 24.  That's the nsHTMLDocument change, and it fixes this bug.  The other changes in this bug are:

1)  Tests for this bug, bug 298064, bug 57600. 
2)  Changes to javascript: to do base URIs better.  In particular, newURI now
    stashes away the base URI passed to the newURI call, and we stick that in a
    property on the nsJSChannel.  nsDocument::Reset reads the property.
    There's a bunch of code here because URIs have to support all sorts of
    interfaces, sadly, and I had to implement a new URI class for javascript:.
3)  A minor change to nsDocumentViewer to fire onload if a cached image is
    loaded directly in the window.  Right now we fire onload if the image is
    not cached, but not if it is, which screwed up the tests.

I have manually verified that bug 296830 and bug 312363 did not regress with this change, but didn't add a test, because I can't reproduce those bugs even backing out the patches that fixed them.  I do think we may want to switch from the subject principal to the GetDocumentFromContext() document principal, now that we enforce same-origin on document.write.  But I'd rather do that in a separate bug; not sure I want to make changes like that on branch.  ;)

This patch incidentally fixes bug 49312 and its various dependencies.

Christian, I'd love it if you take a look at the URI/channel stuff (which is sadly the bulk of the non-test changes).

Comment 29

[Johnny Stenback (:jst)]

Comment on attachment 347446 [details] [diff] [review]
Proposed fix

- In DocumentViewerImpl::LoadComplete():

+  // that one as a success code; otherwise whether we file onload for the image

s/file/fire/

r+sr=jst

Comment 30

[Dave Camp (:dcamp)]

Comment on attachment 347446 [details] [diff] [review]
Proposed fix

>+NS_IMETHODIMP
>+nsJSURI::Read(nsIObjectInputStream* aStream)
>+{
>+    nsresult rv;
>+
>+    rv = aStream->ReadObject(PR_TRUE, getter_AddRefs(mSimpleURI));
>+    if (NS_FAILED(rv)) return rv;
>+
>+    PRBool haveBase;
>+    rv = aStream->ReadBoolean(&haveBase);
>+    if (NS_FAILED(rv)) return rv;
>+
>+    if (haveBase) {
>+        rv = aStream->ReadObject(PR_TRUE, getter_AddRefs(mBaseURI));
>+        if (NS_FAILED(rv)) return rv;
>+    }
>+
>+    return NS_OK;
>+}

Shouldn't this be setting up mMutable too?

>+// Use an extra base object to avoid having to manually retype all the
>+// nsIURI methods.  I wish we could just inherit from nsSimpleURI instead.
>+class nsJSURI_base : public nsIURI,
>+                     public nsIMutable
>+{
>+public:
>+    nsJSURI_base(nsIURI* aSimpleURI) :
>+        mSimpleURI(aSimpleURI)
>+    {
>+        mMutable = do_QueryInterface(mSimpleURI);
>+        NS_ASSERTION(aSimpleURI && mMutable, "This isn't going to work out");
>+    }

>+class nsJSURI : public nsJSURI_base,
>+                public nsISerializable,
>+                public nsIClassInfo
>+{

>+    // For use only from deserialization
>+    nsJSURI() : nsJSURI_base() {}

Won't this trigger the assertion?

Comment 31

[Boris Zbarsky [:bzbarsky]]

> Shouldn't this be setting up mMutable too?

Yes.  Will fix.

> Won't this trigger the assertion?

That's calling the no-arg base constructor, which doesn't assert.

Comment 32

[Boris Zbarsky [:bzbarsky]]

Dave, see comment 31.

Comment 33

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 347446 [details] [diff] [review]
Proposed fix

a191b2=beltzner

Comment 34

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to review comments.

Comment 35

[Dave Camp (:dcamp)]

Attachment: test followup

Comment 36

[Dave Camp (:dcamp)]

landed test followup as http://hg.mozilla.org/mozilla-central/rev/cbd14041ff7d

Comment 37

[Boris Zbarsky [:bzbarsky]]

Landed the main patch as http://hg.mozilla.org/mozilla-central/rev/b2fbc259ee2b

Fixed, yay!

Comment 39

[Boris Zbarsky [:bzbarsky]]

So as followups, I'd like to:

1)  Remove nsLocation::FindUsableBaseURI
2)  Change the principal stuff per comment 28.

I'll try to get bugs on that (and tests for bug 49312 plus dependencies) dealt with tomorrow.

Comment 40

[Boris Zbarsky [:bzbarsky]]

Filed bug 465804 on removing FindUsableBaseURI and bug 465806 on the document.open principal thing.

Comment 41

[Boris Zbarsky [:bzbarsky]]

Attachment: roll-up 1.9 patch

This includes both of the diffs in this bug, as well as the one in bug 465752.  When checking in, I need to make sure to land the files diffed here plus the four binary ones (the images)....

Comment 42

[Boris Zbarsky [:bzbarsky]]

I checked in the 4 images into CVS now, so the roll-up patch now accurately reflects what needs to land in 1.9.0.

Comment 44

[Daniel Veditz [:dveditz]]

The work you put into a 1.9.0 patch is much appreciated, but how much risk are we taking on and is a common-enough use case broken to be worth it?

Comment 45

[Boris Zbarsky [:bzbarsky]]

Hmm.  I think the risk is fairly low since we're mostly aligning with IE, and in cases where current interop is poor amongst browsers anyway.  This basically only affects cases when document.write() is called in window A from window B and the two windows have different directories, plus the file:// case this was originally filed on.

Not sure how common it is to use document.write() in file:// URIs, but any such use in a directory other than the one originally loaded in the browser is broken right now.

Comment 46

[Mike Connor [:mconnor]]

Comment on attachment 349196 [details] [diff] [review]
roll-up 1.9 patch

Boris: do you think it's worth taking this late in the 3.0.x cycle?  This feels like a really weird edgecase that's fixed on trunk for those people actually hitting it, which likely isn't many...

Comment 47

[Boris Zbarsky [:bzbarsky]]

Yes, I think it is.  We've had several duplicates of this bug, and this bug breaks at least one HTML tutorial/reference application (see bug 471252).

I should also note that it'll always be "late in the cycle" if a month goes between approval request and anyone looking at the bug.  :(  I'm happy to land this at the beginning of a branch cycle if the approvals actually... y'know... happen.

Comment 48

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 349196 [details] [diff] [review]
roll-up 1.9 patch

Re-setting approval request so I'll actually get mail if it happens.

Comment 49

[Samuel Sidler (old account; do not CC)]

Comment on attachment 349196 [details] [diff] [review]
roll-up 1.9 patch

Yeah, ok. Let's do this...

Approved for 1.9.0.7. a=ss. Please land in CVS when the tree opens for 1.9.0.7 (I'll even send you an email reminder!).

Comment 51

[Boris Zbarsky [:bzbarsky]]

Bug 473003 describes another help system that was broken by this bug, for what it's worth.

Comment 54

[Boris Zbarsky [:bzbarsky]]

Fixed on 1.9.0 branch.

Comment 55

[Al Billings [:abillings - ex-MoCo]]

Verified in 1.9.0.7 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.7pre) Gecko/2009021704 GranParadiso/3.0.7pre.

Verified for 1.9.1 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090213 Shiretoko/3.1b3pre.

Comment 56

[Fraser MacDonald]

I raised a bug 442244 which was marked as a duplicate of this one. Which is resolved. I have just observed my original issue 442244 in Firefox 3.0.10 on Fedora. It might be worth someone verifying that this issue is currently present in Firefox for Linux, and reactivating this one or creating a new issue if it is confirmed. I am not certain of the correct procedure - my excuse for putting this in a comment.

Comment 57

[Boris Zbarsky [:bzbarsky]]

> I am not certain of the correct procedure - my excuse for putting this in a
> comment.

Fraser, correct procedure if this bug is not fixed as far as you can tell is to comment in this bug.  ;)

That said, I just tried Firefox 3.0.10 (installed via yum, so this is the distro build) on my FC9 system, and your testcase from bug 442244 works fine for me.  If it also does for you but there's another testcase that shows the problem, could you please file a new bug with the new testcase and cc me?  Alternately, if you're using some other Firefox 3.0.10 than the one yum installs, let me know which one it is so I can make sure to test the same thing that you're testing?

Comment 58

[Fraser MacDonald]

Boris - apologies. I messed up the install of Firefox 3.0.10. I re-installed through yum and the test case works correctly. I unfortunately don't use Linux often - so am a little clumsy with it. Thanks :)