Split nsIContentPolicy::TYPE_OBJECT into TYPE_EMBED and TYPE_OBJECT so that Request.context can reflect the correct value

RESOLVED FIXED in Firefox 42

Status

()

Core
DOM
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

({dev-doc-needed})

unspecified
mozilla42
x86
Mac OS X
dev-doc-needed
Points:
---

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

3 years ago
RequestContext supports both "embed" and "object", so we need to split this content policy type in order to support both of those.
(Assignee)

Updated

3 years ago
Assignee: nobody → ehsan
(Assignee)

Comment 1

3 years ago
Created attachment 8584804 [details] [diff] [review]
Split nsIContentPolicy::TYPE_OBJECT into TYPE_EMBED and TYPE_OBJECT so that Request.context can reflect the correct value
Attachment #8584804 - Flags: review?(bugs)
(Assignee)

Comment 2

3 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.
Attachment #8584804 - Attachment is obsolete: true
Attachment #8584804 - Flags: review?(bugs)
(Assignee)

Comment 3

3 years ago
Created 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

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.
Attachment #8585070 - Flags: review?(bugs)

Comment 4

3 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.
Attachment #8585070 - Flags: review?(bugs) → review+
(Assignee)

Comment 5

3 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>
Flags: needinfo?(ehsan)

Comment 6

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

Comment 7

3 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!
Flags: needinfo?(ehsan)
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
(Assignee)

Comment 9

3 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?
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
(Assignee)

Comment 11

3 years ago
Created attachment 8635722 [details] [diff] [review]
Correctly reflect object and embed RequestContext values

Note that these elements cannot be intercepted for now.  See bug
1168676 for the background.
Attachment #8585070 - Attachment is obsolete: true
Attachment #8635722 - Flags: review?(bugs)
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.
Attachment #8635722 - Flags: review?(bugs) → review+
(Assignee)

Comment 13

3 years ago
Sure, I'll add a method.
https://hg.mozilla.org/mozilla-central/rev/dacb212f8c41
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
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!
Flags: needinfo?(ehsan)
Keywords: #relman/triage/defer-to-group, dev-doc-needed
(Assignee)

Comment 17

3 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.
Flags: needinfo?(ehsan)
(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?
Flags: needinfo?(ehsan)
(Assignee)

Comment 19

3 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...
Flags: needinfo?(ehsan)
You need to log in before you can comment on or make changes to this bug.