564059 - JEP 115 - Content frame implementation

Status: RESOLVED WONTFIX

(Add-on SDK Graveyard :: General, defect)

Description

[Dietrich Ayala (:dietrich)]

Making a bug to track this work, since it's going to be a module, not just a loose framework for other modules to follow.

Making it a module guarantees centralized and consistent access to unprivileged (aka dangerous) content.

I'll take the feedback from Felipe on the forum thread and post an implementation here.

Comment 1

[Dietrich Ayala (:dietrich)]

Attachment: v1

First cut at an implementation of the approach Brian and I talked about on IRC, and that I posted about in the forum.

Two of Felipe's comments that aren't specifically addressed in the patch:

* CSS: I'm exposing each DOM piece that would need to be styled, for now. Once we get further along we can figure out if it needs to be more restrictive, and make a decision about passing CSS then.

* How this works with the module.add(module.ctor()) pattern - there's nothing that the content-frame object itself needs to be added to, so I think that pattern doesn't apply here.

Comment 2

[Dietrich Ayala (:dietrich)]

To clarify, just asking for feedback on the approach.

TODO:
- there's a leak
- implement unloading
- scriptability toggle
- docs

Comment 3

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 443806 [details] [diff] [review]
v1

If I understand this correctly, it provides a low-level API that high-level APIs like Panel, Widget, and Page Worker will use to create iframes into which they load content specified by addons, and to which they provide access via their APIs.  So Frame itself is never exposed to consumers of the high-level APIs; rather, a subset of its properties is exposed as properties on the high-level APIs, which then convey property accesses to the underlying Frame.

That seems like a fine plan to me!  When Brian, Daniel, and I initially hashed out the Frame object in JEP 115, we had a reason for exposing it the way we did, but I don't recall what it was now, and looking at this implementation, I prefer its flatter property space, because it simplifies the high-level APIs for addon developers.  It may complicate the implementation of those APIs somewhat, but the interface wins are well worth that price.

Regarding URL detection, on the other hand, in conversations with Brian, we came to the conclusion that it would be better to require URLs to be explicitly flagged by wrapping them in a URL() constructor, despite the additional API complexity, as that eliminates the risk of an addon specifying content it retrieved from a third party that it thinks is HTML but which is parseable as a URL (whether accidentally or maliciously) and thus unexpectedly causes the URL to get loaded.  The URL constructor would be provided by a very simple API and initially expose only a toString() method (and, of course, the ability to introspect instances to determine that they are instances of URL).

I'm fine with the way this module uses the document/window/contentDocument/contentWindow terms, as they are consistent with the usage of those terms in the chrome containers this API is exposing.  However, in the high-level APIs that expose only the content's "document" and "window" objects, we should name those just "document" and "window", as those names are simpler, and it is not necessary in those APIs to distinguish between them and their chrome container equivalents.

The "onLoad" property should be called "onReady", as it is handling the "DOMContentLoaded" event, which we should distinguish from the "load" event, and "ready" is the common simpler synonym for "DOMContentLoaded" in JavaScript libraries like jQuery.

Also, nit: I would probably call "node" something more descriptive of its role, such as "container" or "parent".

I defer to Brian on the "script" property.  As I recall, he wanted there to be a more complex value in order that it be able to convey additional security characteristics in the future besides simply whether or not the iframe can execute script.  Hence in his proposal there is an "allow" hash for which "script" is but one property (and script itself may eventually be more complex than just a boolean).

However it is exposed, though, implementation of the property is trivial.  Just do nothing if script is enabled and set iframe.docShell.allowJavascript to false if it is disabled.

Finally, as a low-level API, it's not clear why the implementation uses closures to hide private properties, although it certainly doesn't hurt to do so!

Comment 4

[Myk Melez [:myk] [@mykmelez]]

One more thing!  The "content" property should be optional and settable after construction, so a consumer can create a frame and later load content into it by setting its content property:

  let frame = Frame();

  function onSomethingHappened() {
    frame.content = "<content to load>";
  }

Comment 5

[Dietrich Ayala (:dietrich)]

Not working on this atm, and not actually sure it's needed now, given the e10s changes.

Comment 6

[Myk Melez [:myk] [@mykmelez]]

The Add-on SDK is no longer a Mozilla Labs experiment and has become a big enough project to warrant its own Bugzilla product, so the "Add-on SDK" product has been created for it, and I am moving its bugs to that product.

To filter bugmail related to this change, filter on the word "looptid".

Comment 7

[Myk Melez [:myk] [@mykmelez]]

Seems like we decided to do something different in the end.