827160 - Implement HTMLObjectElement.typeMustMatch

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Comment 1

[Sebastian Wong]

I'd love to work on this. Could you tell me how I should go about getting started on this?

Comment 2

[John Schoenick [:johns]]

(In reply to Sebastian Wong from comment #1)
> I'd love to work on this. Could you tell me how I should go about getting
> started on this?

Hey Sebastian! If you haven't, you should read this to get setup building firefox and understanding the basic procedure for submitting patches: https://developer.mozilla.org/en-US/docs/Introduction

So for this bug, we would want to follow the spec[1] as closely as possible -- but note that we aren't currently 100% in agreement with the spec, so there may be some discrepancies. Feel free to ask if you aren't sure on how something should behave.

The code for the Object tag is controlled by content/html/content/src/HTMLObjectElement, which inherits most of its behavior from content/base/src/nsObjectLoadingContent

First, we would need to add typeMustMatch to HTMLObjectElement. It looks like it was included here, commented out:
https://mxr.mozilla.org/mozilla-central/source/dom/webidl/HTMLObjectElement.webidl

Uncommenting that should generate the necessary methods on HTMLObjectElement, which would need to be stubbed out (see Declare() and SetDeclare() for a similar bool attribute):
https://mxr.mozilla.org/mozilla-central/source/content/html/content/src/HTMLObjectElement.h

Then, we need to patch nsObjectLoadingContent to actually follow the typeMustMatch logic, around here:
https://mxr.mozilla.org/mozilla-central/source/content/base/src/nsObjectLoadingContent.cpp#1411

in nsObjectLoadingContent, the codepath we're concerned with works like this;

1) LoadObject() is called when the object is created, and calls UpdateObjectParameters()
2) UpdateObjectParamaters() determines we need to open the channel (the data URI in this case) to determine the type
3) It sets mType to eType_Loading, and LoadObject responds by opening a channel.
4) OnStartRequest is called when the channel opens, checks that the channel is valid, and calls LoadObject() again
5) LoadObject calls UpdateObjectParameters again. UpdateObjectParameters has a channel this time, and determines mType = eType_Plugin.
6) LoadObject responds by loading the plugin

So at (4), around line 1411 in nsObjectLoadingContent, we need to adjust our type determination to check if we have a typeMustMatch parameter, and, if true, refuse to continue the load of the channel type does not equal the explicit type. UpdateObjectParameters has a stateInvalid boolean it uses to track this, so setting that to true would properly refuse the load at this point.

Let me know if this makes sense. You can also jump in IRC on irc.mozilla.org #introduction, which is a great place to get help working on mozilla code. Feel free to also ping me as johns on IRC.

Comment 3

[Mina Almasry]

Hi I'd love to work on this, especially since went into such detail explaining the bug. Please let me know if this is available, and assign if possible.

Thanks!

Comment 4

[Sebastian Wong]

I'm still working on this. Progress has been rather slow because I've been busy doing an internship.

Comment 5

[Sebastian Wong]

Hi John, 

I'm unsure as to how I can go about checking for the existence of a typeMustMatch parameter since HTMLObjectElement inherits from nsObjectLoadingContent. Is there a boolean equivalent of GetAttr() that I should be using? Please advise.

Comment 6

[Josh Matthews [:jdm]]

HasAttr exists, if that's what you mean.

Comment 7

[John Schoenick [:johns]]

(In reply to Sebastian Wong from comment #5)
> Hi John, 
> 
> I'm unsure as to how I can go about checking for the existence of a
> typeMustMatch parameter since HTMLObjectElement inherits from
> nsObjectLoadingContent. Is there a boolean equivalent of GetAttr() that I
> should be using? Please advise.

HasAttr is what you want I think, see for instance:
http://dxr.mozilla.org/mozilla-central/source/content/base/src/nsObjectLoadingContent.cpp#l1398

Comment 8

[Sebastian Wong]

Attachment: Progress so far

Attached is the changes I have on my machine so far. To test, I created a html file with the following object : 

<object type="video/quicktime" data="helloworld.swf" width="350" height="95" typemustmatch="typemustmatch"></object>

and the HasAttr check for nsGkAtoms::typeMustMatch appears to return false. Is the way I added the nsGkAtom wrong? Or am I testing the typeMustMatch check wrongly

Comment 9

[Boris Zbarsky [:bzbarsky]]

> Is the way I added the nsGkAtom wrong?

If you want to use it for an HTML attribute name, yes.  All attribute names are ASCII-lowercase in HTML by the time they become atoms.

Comment 10

[Sebastian Wong]

Attachment: 827160.patch

code now recognizes typemustmatch attribute in object elements. I'm also not completely sure what I have for the content type checking is correct.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 772659 [details] [diff] [review]
827160.patch

>+    if(mContentType.Equals(NS_ConvertUTF16toUTF8(typeAttr).get())) {

Drop the .get() bit.

>+    SetBoolAttr(nsGkAtoms::typemustmatch, aValue);

You want SetHTMLBoolAttr (and the property should be SetterThrows in the IDL).

Comment 12

[John Schoenick [:johns]]

Comment on attachment 772659 [details] [diff] [review]
827160.patch

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

::: content/base/src/nsObjectLoadingContent.cpp
@@ +1440,5 @@
> +    thisContent->GetAttr(kNameSpaceID_None, nsGkAtoms::type, typeAttr);
> +    if(mContentType.Equals(NS_ConvertUTF16toUTF8(typeAttr).get())) {
> +      stateInvalid = true;
> +    }
> +  }

This isn't quite right -- mContentType hasn't been set yet. This function determines what it should be and sets it at the end.

The spot we want to add this check is around line 1612 [1]. This is the logic related to whether to use the channel type instead of the guessed-type. It currently doesn't take typeMustMatch into account, but would need to be tweaked to use logic as described in the spec here: http://www.whatwg.org/specs/web-apps/current-work/multipage/the-iframe-element.html#attr-object-typemustmatch

[1] http://dxr.mozilla.org/mozilla-central/source/content/base/src/nsObjectLoadingContent.cpp#l1612

Comment 13

[Sebastian Wong]

Attachment: Moved typemustmatch logic check

How does this look?

Comment 14

[John Schoenick [:johns]]

Comment on attachment 773050 [details] [diff] [review]
Moved typemustmatch logic check

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

::: content/base/src/nsObjectLoadingContent.cpp
@@ +1615,5 @@
> +      thisContent->GetAttr(kNameSpaceID_None, nsGkAtoms::type, typeAttr);
> +      if (!newMime.Equals(NS_ConvertUTF16toUTF8(typeAttr))) {
> +        stateInvalid = true;
> +      }
> +    } else if (typeHint == eType_Plugin) {

This still isn't quite right, newMime at this point would already be the type attribute, and only adopts the channel's type below if overrideChannelType is false (L1644). We need to compare newMime (which is also mOriginalContentType at this point) with channelType, but ensure that we're doing so in the right order as defined by the spec:

http://www.whatwg.org/specs/web-apps/current-work/multipage/the-iframe-element.html#attr-object-typemustmatch

See step 4.7.2, which is what the logic at this part of the code is approximating. (We should also update the comment above that is explaining the steps to reflect this)

Comment 15

[Sebastian Wong]

Attachment: 827160.patch

Comment 16

[John Schoenick [:johns]]

Comment on attachment 777109 [details] [diff] [review]
827160.patch

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

Thanks for working on this -- this looks pretty good now with a few tweaks requested below.

This is also going to need tests written at some point, are you interested in learning about Mochitest and writing a testcase for this?

::: content/base/src/nsObjectLoadingContent.cpp
@@ +1596,5 @@
>                            eType_Null : GetTypeOfContent(newMime);
>  
>      //
>      // In order of preference:
> +    // 1) Perform typemustmatch check and set stateInvalid if check fails

This should note that if we have typemustmatch and the check succeeds, we use that type without doing any other steps.

@@ +1611,5 @@
>  
>      bool overrideChannelType = false;
> +    if (thisContent->HasAttr(kNameSpaceID_None, nsGkAtoms::typemustmatch)) {
> +      nsAutoString typeAttr;
> +      thisContent->GetAttr(kNameSpaceID_None, nsGkAtoms::type, typeAttr);

Getting the type attr a second time in this function is a bit redundant, since we already grabbed it at the "// Initial MIME Type" section above. Can we move typeAttr up to be declared next to newMime, then in the initial mime type section do:

> nsAutoString rawTypeAttr;
> GetAttr(...)
> CopyUTF16t6oUTF8(rawTypeAttr, typeAttr)
> newMime = typeAttr

Then we don't need to get our type a second time here or worry about cases where we touch newMime in between.

@@ +1612,5 @@
>      bool overrideChannelType = false;
> +    if (thisContent->HasAttr(kNameSpaceID_None, nsGkAtoms::typemustmatch)) {
> +      nsAutoString typeAttr;
> +      thisContent->GetAttr(kNameSpaceID_None, nsGkAtoms::type, typeAttr);
> +      if(!typeAttr.IsEmpty() && !newMime.Equals(channelType)) {

No need to check IsEmpty, if HasAttr() passes and type="" then we should still enforce that (which will fail, but that's the point)

This should also use EqualsIgnoreCase() per the spec.

Comment 17

[Sebastian Wong]

Attachment: 827160.patch

Comment 18

[Sebastian Wong]

I'd love to learn about Mochitest. Would the testcase be part of the patch or would it be a separate submission?

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

It can be separate patch too, but usually it is easier if the the code change and test for it
forms one patch.

Comment 20

[John Schoenick [:johns]]

Comment on attachment 782909 [details] [diff] [review]
827160.patch

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

This looks good, newMime.EqualsIgnoreCase() should be using typeAttr, and a few nits -- r+ with those fixed.

As for tests, they can be in this patch or a separate one, but I think they should be ready before we land this.

See: https://developer.mozilla.org/en-US/docs/Mochitest

For a test that tests plugin tags, see for example:
http://dxr.mozilla.org/mozilla-central/source/content/base/test/test_bug810494.html

You can use the same SpecialPowers tool used there to test what type a tag has loaded, for example to see if the tag loaded a plugin (instead of nothing):
> const OBJLC = SpecialPowers.Ci.nsIObjectLoadingContent;
> SpecialPowers.wrap(document.getElementById("myplugin")).displayedType == OBJLC.TYPE_PLUGIN;

For faking the server's content-type you can cheat and use data URIs:

> <!-- Should load test plugin application/x-test -->
> <object data="data:application/x-test,foo"></object>
> <!-- Should load test plugin application/x-test2, ignoring type="" -->
> <object type="application/x-test" data="data:application/x-test2,foo"></object>
> <!-- Should load nothing, channel type does not match type and typeMustMatch is present -->
> <object type="application/x-test" data="data:application/x-test2,foo" typeMustMatch></object>
> ... etc ...

The test should go in content/base/test and be added to Makefile.in there. Let me know if you need any help with this

::: content/base/src/nsObjectLoadingContent.cpp
@@ +1353,5 @@
>    LOG(("OBJLC [%p]: Updating object parameters", this));
>  
>    nsresult rv;
>    nsAutoCString newMime;
> +  nsAutoCString typeAttr;

This should be an nsAutoString -- CString is a 8-bit string type, String is 16-bit. The CopyUTF16toUTF8 call below will convert it to UTF8 for newMime, which is a CString.

@@ +1636,4 @@
>  
>      bool overrideChannelType = false;
> +    if (thisContent->HasAttr(kNameSpaceID_None, nsGkAtoms::typemustmatch)) {
> +      if(!newMime.EqualsIgnoreCase(channelType.get())) {

nit: space between if and the parenthesis.

I think we want to be testing the typeAttr var you added here rather than newMime -- other logic above might've fussed with newMime, but typeMustMatch should check strictly against the type attribute.

::: dom/webidl/HTMLObjectElement.webidl
@@ +18,5 @@
>    [Pure, SetterThrows]
>             attribute DOMString data;
>    [Pure, SetterThrows]
>             attribute DOMString type;
> +  [SetterThrows]

I believe this can be marked Pure as well, see https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#.5BPure.5D

Comment 21

[Sebastian Wong]

Attachment: 827160.patch

Added mochitests and changed typeattr logic slightly

Comment 22

[Sebastian Wong]

Attachment: 827160.patch

Minor changes from previous submission

Comment 23

[John Schoenick [:johns]]

Comment on attachment 798014 [details] [diff] [review]
827160.patch

bz, could you doublecheck the webidl change here? (does this need sr since it's exposing a new attribute?)

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 798014 [details] [diff] [review]
827160.patch

>+      if (!typeAttr.EqualsIgnoreCase(channelType.get())) {

I guess this works, but it's very non-obvious that this only does ASCII case-folding...  LowerCaseEqualsAscii might be better since channelType is guaranteed lowercase.

The WebIDL bits look great.  sr is probably a good thing, but in this case I can just do that (plus, this is just directly implementing the spec, and the spec seems reasonable).

Comment 25

[John Schoenick [:johns]]

Comment on attachment 798014 [details] [diff] [review]
827160.patch

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

One last r- for a few tweaks to the test, but otherwise this looks good to go. Thanks again for working on this!

Once you have these tweaks in, r? me one last time, and set sr? (super-review) to :bz so he can approve exposing the new attribute.

::: content/base/src/nsObjectLoadingContent.cpp
@@ +1661,4 @@
>  
>      bool overrideChannelType = false;
> +    if (thisContent->HasAttr(kNameSpaceID_None, nsGkAtoms::typemustmatch)) {
> +      if (!typeAttr.EqualsIgnoreCase(channelType.get())) {

We should switch to LowerCaseEqualsASCII here as :bz suggested

::: content/base/test/test_bug827160.html
@@ +17,5 @@
> +<object id="shouldLoad" data="data:application/x-test,foo"></object>
> +<!-- Should load test plugin application/x-test2, ignoring type="" -->
> +<object id="shouldIgnoreType" type="application/x-test" data="data:application/x-test2,foo"></object>
> +<!-- Should load nothing, channel type does not match type and typeMustMatch is present -->
> +<object id="shouldNotLoad" type="application/x-test" data="data:application/x-test2,foo" typemustmatch=""></object>

You don't need the ="" here, for attributes you like this you can just do
> <object type="a" data="data:foo" typemustmatch>

Can you also add a few more test cases here with typemustmatch set, but one with type missing, and one with data missing? These don't make much sense logically, but we should make sure the code does the right thing. (type missing should not load, data missing should)

@@ +20,5 @@
> +<!-- Should load nothing, channel type does not match type and typeMustMatch is present -->
> +<object id="shouldNotLoad" type="application/x-test" data="data:application/x-test2,foo" typemustmatch=""></object>
> +<pre id="test">
> +<script type="application/javascript;">
> +

This script should be wrapped in a

> document.addEventListener("load", function() {
>  ...
> }, false);

Block -- the object tags aren't guaranteed to be loaded as soon as the script runs, so we don't want the test to randomly start failing later if that changes.

@@ +24,5 @@
> +
> +const OBJLC = SpecialPowers.Ci.nsIObjectLoadingContent;
> +test1 = SpecialPowers.wrap(document.getElementById("shouldLoad")).displayedType == OBJLC.TYPE_PLUGIN;
> +test2 = SpecialPowers.wrap(document.getElementById("shouldIgnoreType")).displayedType == OBJLC.TYPE_PLUGIN;
> +test3 = SpecialPowers.wrap(document.getElementById("shouldNotLoad")).displayedType != OBJLC.TYPE_PLUGIN;

This should check |== OBJLC.TYPE_NULL| for sanity's sake

@@ +29,5 @@
> +
> +//tests
> +is(test1, true, "Testing object load without type, failed expected load");
> +is(test2, true, "Testing object load with type, failed expected load");
> +is(test3, true, "Testing object load with typemustmatch, load success even though failure expected");

Here you can just do
> is(SpecialPowers.wrap(document.getElementById("shouldLoad")).displayedType, OBJLC.TYPE_PLUGIN, "Testing object load without type, failed expected load")

Comment 26

[Boris Zbarsky [:bzbarsky]]

sr=me, fwiw.

Comment 27

[John Schoenick [:johns]]

Hey Sebastian, any progress on this? If you're busy I would happy to do the remaining cleanup and get this landed

Comment 28

[Sebastian Wong]

Attachment: 827160.patch

Sorry for the delay. Interview season and school is a little overwhelming. I've made the changes you requested (Or at least attempted to). I tried placing the  "asserts directly into the document.addEventListener and the unit tester didn't seem to pick up the assertions. Moving the assertions off into a separate function seemed to solve the issue. Should the attached patch not work for you, please go ahead and do whatever is necessary to land the patch. Again, sincerest apologies.

Comment 29

[John Schoenick [:johns]]

Comment on attachment 816838 [details] [diff] [review]
827160.patch

This looks good to me. I made a few tweaks to the test to account for click-to-play and the Makefile.in -> manifest move.

Pushed to try to ensure the new test doesn't encounter issues anywhere:
https://tbpl.mozilla.org/?tree=Try&rev=3f2f918abbf0

Comment 30

[John Schoenick [:johns]]

Attachment: added HTMLObjectElement typemustmatch check as well as unit tests. r=johns,sr=bz

Comment 31

[John Schoenick [:johns]]

Comment on attachment 819999 [details] [diff] [review]
added HTMLObjectElement typemustmatch check as well as unit tests. r=johns,sr=bz

sr=bz per comment 26

Fixed minor issue with activating second test plugin:
https://tbpl.mozilla.org/?tree=Try&rev=f40540fb28ae

Pushed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9349b9b4b22c

Comment 32

[John Schoenick [:johns]]

Followup, disable the included test on platforms that don't support plugins:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5dd1dc2ae18

Comment 33

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9349b9b4b22c
https://hg.mozilla.org/mozilla-central/rev/f5dd1dc2ae18

Comment 34

[John Schoenick [:johns]]

(In reply to Wes Kocher (:KWierso) from comment #33)
> https://hg.mozilla.org/mozilla-central/rev/9349b9b4b22c
> https://hg.mozilla.org/mozilla-central/rev/f5dd1dc2ae18

@Sebastian - this should be in FF27 if nothing stops it, thanks for helping with this!

Comment 35

[Jean-Yves Perrier [:teoli]]

Doc updated:
https://developer.mozilla.org/en-US/docs/Web/API/HTMLObjectElement
https://developer.mozilla.org/en-US/docs/Web/API/HTMLObjectElement.typeMustMatch
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/object
https://developer.mozilla.org/en-US/Firefox/Releases/27