1148044 - Split nsIContentPolicy::TYPE_SUBDOCUMENT into TYPE_FRAME and TYPE_IFRAME so that Request.context can reflect the correct value

Status: RESOLVED FIXED

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

Description

[(no longer active)]

Request.context can reflect "frame" and "iframe" separately, so we should split TYPE_SUBDOCUMENT into two for this to be reflected correctly.

Comment 1

[(no longer active)]

Attachment: Split nsIContentPolicy::TYPE_SUBDOCUMENT into TYPE_FRAME, TYPE_IFRAME and TYPE_IMPORT so that Request.context can reflect the "frame", "iframe" and "import" values correctly

Note that I had to touch TYPE_IMPORT because HTML Imports used to be
loaded as TYPE_SUBDOCUMENT.  I also tried to do the right thing for
imports where treating them as frames doesn't make sense.

Comment 2

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

Comment on attachment 8584840 [details] [diff] [review]
Split nsIContentPolicy::TYPE_SUBDOCUMENT into TYPE_FRAME, TYPE_IFRAME and TYPE_IMPORT so that Request.context can reflect the "frame", "iframe" and "import" values correctly

This is guaranteed to break _lots_ of addons.
Please land *after* the next merge and contact addon authors.

You may want to ask also dveditz about these changes.

(don't land any of these nsIContentPolicy changes before the merge!)

But this patch make loading a document into <object> rather odd. You get
nsIContentPolicy::TYPE_FRAME as the contentType. Because of that r-

Comment 3

[(no longer active)]

Dan, Jorge, as part of the implementation of service workers, we need to expose the context for the intercepted Request objects inside the service worker.  The context attribute <https://fetch.spec.whatwg.org/#concept-request-context> gives the service worker information about where a particular fetch has been initiated from.  The implementation of this is based on nsIContentPolicy types.

In order to do this, we need to split some of the existing types into multiple subtypes, and this has addon-compat concerns.  I will land these changes after the next uplift, but I'd like to give you a heads up and see if this is OK.  Next week, I will write a blog post about this and we can use that when publicizing this among the add-on developers.

Comment 4

[(no longer active)]

(Note the discussion that is happening in bug 1148935 right now.  It seems like we will not modify nsContentPolicyTypes after all...)

Comment 5

[Jorge Villalobos [:jorgev] (he/him)]

The impact would be significant, but I don't think it would be bigger than other changes we've done before. Major add-ons like NoScript and Adblock Plus would certainly adapt to the new scheme quickly. Others might take longer.

But if there are viable alternatives that don't break add-ons, I'm all for those :)

Comment 6

[Daniel Veditz [:dveditz]]

Adding new request types is easy, splitting existing ones is likely to cause all kinds of problems with broken security decisions. I'm particularly worried about the script ones in the other bug.

Comment 7

[(no longer active)]

The exact final decision is in flux still, but we will definitely avoid splitting the existing ones...

Comment 8

[(no longer active)]

Attachment: Correctly reflect frame and iframe RequestContext values

Comment 9

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

Comment on attachment 8633043 [details] [diff] [review]
Correctly reflect frame and iframe RequestContext values

What about <object> ?

Comment 10

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

Comment on attachment 8633043 [details] [diff] [review]
Correctly reflect frame and iframe RequestContext values

I'd say treating <object> as nsIContentPolicy::TYPE_INTERNAL_FRAME is even more wrong than nsIContentPolicy::TYPE_INTERNAL_IFRAME.

(But don't we actually want separate thing for it)

Comment 11

[(no longer active)]

Comment on attachment 8633043 [details] [diff] [review]
Correctly reflect frame and iframe RequestContext values

I will deal with <object> in bug 1148030.

Comment 12

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

Comment on attachment 8633043 [details] [diff] [review]
Correctly reflect frame and iframe RequestContext values

So we'll need to have two internal types for <object>? One for subdocuments, and one for the rest.
Might be good to not land this before <object> is also fixed, though I guess
<object> used as <iframe> is rather rare case.

Comment 13

[(no longer active)]

I'm working on the other bug now, so this can wait!

Comment 14

[(no longer active)]

So, more object the case of object.  We disabled interception of embed/object in bug 1168676 because in the case where object would create a nested browsing context, so the issue in comment 12 doesn't really exist right now.  I'm still going to finish the work in bug 1148030 just in the interest of putting the work into the tree, but it will be disabled for now.

Once we decide to intercept object/embed, we would need to treat the case where they create a nested browsing context somehow, and as part of that we need to decide what RequestContext to send in that case, and then we can decide whether we're going to need a new internal type for those or not.

Comment 15

[Pulsebot]

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

Comment 16

[Carsten Book [:Tomcat]]

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