Closed Bug 1148044 Opened 9 years ago Closed 9 years ago

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

Categories

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

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)

References

Details

(Keywords: addon-compat, dev-doc-needed)

Attachments

(1 file, 1 obsolete file)

Request.context can reflect "frame" and "iframe" separately, so we should split TYPE_SUBDOCUMENT into two for this to be reflected correctly.
Assignee: nobody → ehsan
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.
Attachment #8584840 - Flags: review?(bugs)
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-
Attachment #8584840 - Flags: review?(bugs) → review-
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.
Flags: needinfo?(jorge)
Flags: needinfo?(dveditz)
Keywords: dev-doc-needed
(Note the discussion that is happening in bug 1148935 right now.  It seems like we will not modify nsContentPolicyTypes after all...)
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 :)
Flags: needinfo?(jorge)
Keywords: addon-compat
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.
Flags: needinfo?(dveditz)
The exact final decision is in flux still, but we will definitely avoid splitting the existing ones...
Attachment #8584840 - Attachment is obsolete: true
Attachment #8633043 - Flags: review?(bugs)
Comment on attachment 8633043 [details] [diff] [review]
Correctly reflect frame and iframe RequestContext values

What about <object> ?
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)
Attachment #8633043 - Flags: review?(bugs) → review-
Comment on attachment 8633043 [details] [diff] [review]
Correctly reflect frame and iframe RequestContext values

I will deal with <object> in bug 1148030.
Attachment #8633043 - Flags: review- → review?(bugs)
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.
Attachment #8633043 - Flags: review?(bugs) → review+
I'm working on the other bug now, so this can wait!
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.
https://hg.mozilla.org/mozilla-central/rev/ad97d9bfc686
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: