Closed Bug 667949 Opened 13 years ago Closed 13 years ago

Content scripts that pass addEventListener() an object supporting the EventListener interface don't work

Categories

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

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: avarma, Assigned: ochameau)

References

()

Details

Attachments

(2 files)

When passing a function to addEventListener(), the function is given an event object whose 'target' property is properly proxied/wrapped. However, when passing an EventListener-style object to addEventListener(), its handleEvent() method is passed an event object whose 'target' property is *not* properly proxied/wrapped.

Here's a trivial example content script that exhibits the bug:

var myListener = {
  handleEvent: function(event) {
    // This produces e.g. [object XrayWrapper [object HTMLHtmlElement]],
    // instead of [object HTMLHtmlElement].
    console.log(event.target);
  }
};

document.addEventListener("mouseover", myListener, true);
Assignee: nobody → poirot.alex
OS: Mac OS X → All
Priority: -- → P3
Hardware: x86 → All
Target Milestone: --- → 1.1
Status: NEW → ASSIGNED
Attached file Pull request 207
The patch is in good shape, I need one more cycle before requesting review.

Otherwise, I think that this bug require an higher priority as it may not be limited to such addEventListener uses.
Comment on attachment 544800 [details]
Pull request 207

I had to introduce another kind of proxy.
So as content proxies becomes hairy, I wrote a documentation for them with ASCII diagrams, examples, stacktraces, ...

Irakli: About the review, I started to address your previous review comments from bug 601295, on another branch:
  https://github.com/ochameau/addon-sdk/commits/cs-proxy
Unfortunately, these changes are out of date and may not be applied on current master.
So if it still make sense to address them I may update these old commits after this patch has been applied.
Attachment #544800 - Flags: review?(rFobic)
Attachment #544800 - Flags: review?(wbamberg)
Will: Can you review documentation I provided about content script proxies?
I wrote this documentation to help reviewers and sdk team to understand these proxies.
But if you think that it makes sense to highlight such documentation to addon developers, feel free to propose necessary modifications.
(For example Irakli suggested to use the work "wrapper" instead of "proxy", it makes more sense for the end user of proxies, but less for developers of these proxies)
Otherwise, I think this doc may help you write better description of content scripts wrappers in high level API!
Attachment #544800 - Flags: review?(rFobic) → review+
Alex: sure!

I think there should be 2 documents:

1) this one, that includes the internal details of how proxies work, mostly for the SDK team

2) one for add-on developers, that concentrates more on:
    - the fact that access is proxied
    - why access is proxied
    - what this implies for content scripts (i.e., what I can't access from a content script, that I could access from a script loaded into the page)
    - something on unsafeWindow explaining when you actually need it and why you should think 3 times before using it (I'm a bit worried people might hit a problem with content scripts, find out about unsafeWindow, and just start using it, when it's possible that it's a bug in the proxy that ought to be fixed.)

This doc provides invaluable content for (2) and partially overlaps it but it should not, I think, be the same. Having said that I do think its worth having a go at this doc to make it more user-friendly (even for the SDK team!). But pull requests are maybe not the best vehicle for this kind of mass of small changes and reorganizations that I'd have in mind. Either I can redraft it and post it here, and we can iterate on it in this bug, then you can integrate it, or we can close this bug and have a separate bug to redraft the doc. Up to you.
Irakli: I pushed another commit to address your very last comment. Feel free to add another comment on the XrayWrapper proxies documentation.

Will: Yes, I think that having (at least) two distinct documents is the best way to go. Feel free to open a bug to modify other md files. But in anycase, I'd like to land this patch so I'm waiting for your approval/review on this new md file.
So I made some edits to proxy.md, attached. There are a couple of things I'm not very happy with:

- the diagrams: I was going to redraw them to be pretty, but there's some stuff I'm not sure about. In the first diagram, it looks as if the web document's window is calling require("content-proxy").create(...), but it's not, right? That's done by the code in worker.js to set up the sandbox for the content scripts, yes? In the second diagram, I wasn't all that clear about  the XrayWrapper proxy: the text says that they are internal (to what?) and not exposed to content scripts, but the diagram somehow suggestes that they are exposed to content scripts.

- the stack trace is pretty confusing (to me). I tried to explain it in a simpler fashion, but wasn't really successful.

But maybe it's OK, for the time being, for an internal-facing document. It's more important to explain the user-facing side of this at the moment.
(In reply to comment #6)
> Created attachment 548326 [details] [diff] [review] [review]
> Some rework of proxy.md
> 
> 
> So I made some edits to proxy.md, attached. There are a couple of things I'm
> not very happy with:
> 
> - the diagrams: I was going to redraw them to be pretty, but there's some
> stuff I'm not sure about. In the first diagram, it looks as if the web
> document's window is calling require("content-proxy").create(...), but it's
> not, right? That's done by the code in worker.js to set up the sandbox for
> the content scripts, yes? 
Yes, api-utils/lib/content/worker.js grab `window` reference of the document,
and build the `window` object for the content script by calling:
  let contentScriptWindow = require("content-proxy").create(documentWindow);

> In the second diagram, I wasn't all that clear
> about  the XrayWrapper proxy: the text says that they are internal (to
> what?) and not exposed to content scripts, but the diagram somehow suggestes
> that they are exposed to content scripts.
XrayWrapper proxies are internal to content-proxy.js code. They are passed to XrayWrapper objects. XrayWrapper objects and XrayWrapper proxies can't be accessed by the document, nor the content script. But the diagram say that the XrayWrapper proxy has a reference to `myObject`.

> 
> - the stack trace is pretty confusing (to me). I tried to explain it in a
> simpler fashion, but wasn't really successful.
These traces are the finishing touches for code reviewer or code contributor to undertand how this code works. So it is only helpfull for folks that start reading the content-proxy.js file.

> 
> But maybe it's OK, for the time being, for an internal-facing document. It's
> more important to explain the user-facing side of this at the moment.

I've commited your proposal in the pull request. 
I've only replaced XPCNativeWrapper by XrayWrapper. Even if MDC use XPCNativeWrapper, we should stick to XrayWrapper as platform tends to replace XPCNativeWrapper to XrayWrapper.

Will: I'm waiting either r+ on the pull request before landing the whole thing, but feel free to suggest another patch!
Comment on attachment 544800 [details]
Pull request 207

Sorry Alex, I somehow missed this. I think it's fine for now.
Attachment #544800 - Flags: review?(wbamberg) → review+
Landed:
https://github.com/mozilla/addon-sdk/commit/d312a1ecf63a6ef64535e0aa161a078a191bf860
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: