(no longer active) Assignee
	Description • 9 years ago

RequestContext supports both "embed" and "object", so we need to split this content policy type in order to support both of those.

	(no longer active) Assignee
	Comment 2 • 9 years ago

Comment on attachment 8584804 [details] [diff] [review]
Split nsIContentPolicy::TYPE_OBJECT into TYPE_EMBED and TYPE_OBJECT so that Request.context can reflect the correct value

Turns out that applets are also loaded as TYPE_OBJECT, so we need to add TYPE_APPLET too.

	(no longer active) Assignee
	Comment 3 • 9 years ago

Note that TYPE_APPLET is not yet reflected through RequestContext,
pending resolution to this spec issue:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=28360.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 4 • 9 years ago

Comment on attachment 8585070 [details] [diff] [review]
Split nsIContentPolicy::TYPE_OBJECT into TYPE_EMBED, TYPE_OBJECT and TYPE_APPLET so that Request.context can reflect the correct value

I wonder how many addons are broken after all these changes.
You may want to check https://mxr.mozilla.org/addons/ and
perhaps notify the addon authors about the changes.
I wouldn't be surprised if this breaks various ad blockers.

Please land this *after* the next merge, or make sure addon authors are notified about the change asap.

+  case nsIContentPolicy::TYPE_APPLET:
+    mContext = RequestContext::Internal;
looks odd. Why we don't have a proper type for applets there? 
Spec issue? Please file a spec bug and then a followup to fix Gecko's behavior.

	(no longer active) Assignee
	Comment 5 • 9 years ago

(In reply to Olli Pettay [:smaug] from comment #4)
> Comment on attachment 8585070 [details] [diff] [review]
> Split nsIContentPolicy::TYPE_OBJECT into TYPE_EMBED, TYPE_OBJECT and
> TYPE_APPLET so that Request.context can reflect the correct value
> 
> I wonder how many addons are broken after all these changes.
> You may want to check https://mxr.mozilla.org/addons/ and
> perhaps notify the addon authors about the changes.
> I wouldn't be surprised if this breaks various ad blockers.
> 
> Please land this *after* the next merge, or make sure addon authors are
> notified about the change asap.

OK, needinfoing myself to remember...

> +  case nsIContentPolicy::TYPE_APPLET:
> +    mContext = RequestContext::Internal;
> looks odd. Why we don't have a proper type for applets there? 
> Spec issue? Please file a spec bug and then a followup to fix Gecko's
> behavior.

Yes, spec issue that I mentioned in the commit message: <https://www.w3.org/Bugs/Public/show_bug.cgi?id=28360>

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 6 • 9 years ago

(I never read multiline commit messages. All I want is the bug number and short description of the change)

	(no longer active) Assignee
	Comment 7 • 9 years ago

(In reply to Olli Pettay [:smaug] from comment #6)
> (I never read multiline commit messages. All I want is the bug number and
> short description of the change)

Fair!

	John Schoenick [:johns]
	Comment 8 • 9 years ago

We never perform resource loads for applet, and have a further check that any loaded-from-channel plugin cannot be java:
 https://dxr.mozilla.org/mozilla-central/source/dom/base/nsObjectLoadingContent.cpp#1845

Applets (and all plugin loads) will go through shouldProcess(), however

	(no longer active) Assignee
	Comment 9 • 9 years ago

(In reply to John Schoenick [:johns] from comment #8)
> We never perform resource loads for applet, and have a further check that
> any loaded-from-channel plugin cannot be java:
>  https://dxr.mozilla.org/mozilla-central/source/dom/base/
> nsObjectLoadingContent.cpp#1845
> 
> Applets (and all plugin loads) will go through shouldProcess(), however

Well, those requests are are still loaded using TYPE_APPLET, right?

	John Schoenick [:johns]
	Comment 10 • 9 years ago

There is no TYPE_APPLET AFAIK

All <embed>/<applet>/<object> plugin loads are currently TYPE_OBJECT

Java can be loaded in any of those tags

	(no longer active) Assignee
	Comment 11 • 9 years ago

Note that these elements cannot be intercepted for now.  See bug
1168676 for the background.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 12 • 9 years ago

Comment on attachment 8635722 [details] [diff] [review]
Correctly reflect object and embed RequestContext values

>+  // We use TYPE_INTERNAL_OBJECT for applet too, since it is not exposed through
>+  // RequestContext yet.
>+  MOZ_ASSERT(thisContent->IsAnyOfHTMLElements(nsGkAtoms::object, nsGkAtoms::embed, nsGkAtoms::applet));
>+  nsContentPolicyType contentPolicyType = nsIContentPolicy::TYPE_INTERNAL_OBJECT;
>+  if (thisContent->IsHTMLElement(nsGkAtoms::embed)) {
>+    contentPolicyType = nsIContentPolicy::TYPE_INTERNAL_EMBED;
>+  }
why not 
nsContentPolicyType contentPolicyType = thisContent->IsHTMLElement(nsGkAtoms::embed) ?
  nsIContentPolicy::TYPE_INTERNAL_EMBED : nsIContentPolicy::TYPE_INTERNAL_OBJECT;

That way we wouldn't first assign some value, and immediately some other.


>     case eType_Plugin:
>-      objectType = nsIContentPolicy::TYPE_OBJECT;
>+      // We use TYPE_INTERNAL_OBJECT for applet too, since it is not exposed through
>+      // RequestContext yet.
>+      MOZ_ASSERT(thisContent->IsAnyOfHTMLElements(nsGkAtoms::object, nsGkAtoms::embed, nsGkAtoms::applet));
>+      objectType = nsIContentPolicy::TYPE_INTERNAL_OBJECT;
>+      if (thisContent->IsHTMLElement(nsGkAtoms::embed)) {
>+        objectType = nsIContentPolicy::TYPE_INTERNAL_EMBED;
>+      }
ditto



>+  // We use TYPE_INTERNAL_OBJECT for applet too, since it is not exposed through
>+  // RequestContext yet.
>+  MOZ_ASSERT(thisContent->IsAnyOfHTMLElements(nsGkAtoms::object, nsGkAtoms::embed, nsGkAtoms::applet));
>+  nsContentPolicyType contentPolicyType = nsIContentPolicy::TYPE_INTERNAL_OBJECT;
>+  if (thisContent->IsHTMLElement(nsGkAtoms::embed)) {
>+    contentPolicyType = nsIContentPolicy::TYPE_INTERNAL_EMBED;
>+  }
oh, this is in so many places...


might be nicer to have an abstract virtual method in nsObjectLoadingContent returning the right contentpolicytype and implement it
in classes inheriting nsObjectLoadingContent. But either way.

	(no longer active) Assignee
	Comment 13 • 9 years ago

Sure, I'll add a method.

	Pulsebot
	Comment 14 • 9 years ago

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

	Carsten Book [:Tomcat]
	Comment 15 • 9 years ago

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

	Eric Shepherd [:sheppy]
	Comment 16 • 9 years ago

Looking at the current nsIContentPolicyBase.idl, it looks like TYPE_EMBED and TYPE_APPLET have been converted to TYPE_INTERNAL_EMBED and TYPE_INTERNAL_APPLET, which remap to TYPE_OBJECT prior to calling a content policy implementation. I'd like to confirm that this is correct.

Additionally, I'm trying to sort something out. According to this bug, this split was implemented in mozilla42. But according to bug 1174307, these new type constants were added for mozilla41. Is that correct? If so, how does that work out in real life? Actually, same thing goes for bug 1174307 (splitting out of frame and iframe types internally) -- that too is listed as Firefox 42 despite the constants being added in Firefox 41, according to this bug.

With that assumption made, I've updated https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIContentPolicy, as well as Firefox 41 for developers. Please advise on the questions above so I can be sure these are all documented just right.

Thanks!

	(no longer active) Assignee
	Comment 17 • 9 years ago

(In reply to Eric Shepherd [:sheppy] from comment #16)
> Looking at the current nsIContentPolicyBase.idl, it looks like TYPE_EMBED
> and TYPE_APPLET have been converted to TYPE_INTERNAL_EMBED and
> TYPE_INTERNAL_APPLET, which remap to TYPE_OBJECT prior to calling a content
> policy implementation. I'd like to confirm that this is correct.

Yes.  IOW, nothing has changed as far as addons are concerned.

> Additionally, I'm trying to sort something out. According to this bug, this
> split was implemented in mozilla42. But according to bug 1174307, these new
> type constants were added for mozilla41. Is that correct?

Yes.

> If so, how does
> that work out in real life? Actually, same thing goes for bug 1174307
> (splitting out of frame and iframe types internally) -- that too is listed
> as Firefox 42 despite the constants being added in Firefox 41, according to
> this bug.

I don't understand what "how does this work in real life?" means.  It means some of those bugs were landed in 41, and others in 42.

> With that assumption made, I've updated
> https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/
> Interface/nsIContentPolicy,

Looks good.  Thanks!

> as well as Firefox 41 for developers. Please
> advise on the questions above so I can be sure these are all documented just
> right.

We don't need to document any of this on those pages.  I removed that from the 41 page, didn't find any other occurrences there or on the 42 page.

	Eric Shepherd [:sheppy]
	Comment 18 • 9 years ago

(In reply to :Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #17)
> (In reply to Eric Shepherd [:sheppy] from comment #16)

> > as well as Firefox 41 for developers. Please
> > advise on the questions above so I can be sure these are all documented just
> > right.
> 
> We don't need to document any of this on those pages.  I removed that from
> the 41 page, didn't find any other occurrences there or on the 42 page.

Why doesn't it need to be included on Firefox 41/42 for developers? It's a change that affects Gecko developers, so it belongs in the section on Gecko and add-on related changes. I know it's internal-only stuff, but it still counts...

As for the issue of parts landing in 41 and parts in 42, it looks as if the constants were added and implemented in 41, but reflecting the correct types back was finished in 42. Does that seem right?

	(no longer active) Assignee
	Comment 19 • 9 years ago

(In reply to Eric Shepherd [:sheppy] from comment #18)
> (In reply to :Ehsan Akhgari (not reviewing patches, not reading bugmail,
> needinfo? me!) from comment #17)
> > (In reply to Eric Shepherd [:sheppy] from comment #16)
> 
> > > as well as Firefox 41 for developers. Please
> > > advise on the questions above so I can be sure these are all documented just
> > > right.
> > 
> > We don't need to document any of this on those pages.  I removed that from
> > the 41 page, didn't find any other occurrences there or on the 42 page.
> 
> Why doesn't it need to be included on Firefox 41/42 for developers? It's a
> change that affects Gecko developers, so it belongs in the section on Gecko
> and add-on related changes. I know it's internal-only stuff, but it still
> counts...

Aren't those pages targeted at Web/add-on developers?  They never mention all of the internal stuff that changed.

> As for the issue of parts landing in 41 and parts in 42, it looks as if the
> constants were added and implemented in 41, but reflecting the correct types
> back was finished in 42. Does that seem right?

No visible behavior from the viewpoint of Web/add-on developers was changed in either 41 or 42.  And with bug 1188062, it will never be exposed for now either.  So it shouldn't really matter which patch landed in which version...