Closed Bug 795275 Opened 7 years ago Closed 7 years ago

Warn about Components removal and add Telemetry

Categories

(Core :: XPConnect, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18
Tracking Status
firefox18 --- fixed
firefox-esr17 18+ fixed

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(5 files, 1 obsolete file)

We should warn, at least for one release, that the Components object is going away.
I think telemetry would be useful here too.
Summary: Warn about Components removal → Warn about Components removal and add Telemetry
A lot of this stuff can be simplified now, and we can stop using the deprecated APIs.
Attachment #665943 - Flags: review?(mrbkap)
We want this right now so that we can avoid the scary warning when content Components
access happens in XBL (which we're allowing going forward). This patch would be overkill
just for that, but I also have plans to introduce a SOW-like protection of the Components
wrapper filtering policy. I can't just do the filename hack for that though, because real-
world XBL filenames might be all over the place. So let's just be safe here.
Attachment #665945 - Flags: review?(mrbkap)
Attachment #665947 - Flags: review?(mrbkap)
Attachment #665948 - Flags: review?(mrbkap)
Comment on attachment 665943 [details] [diff] [review]
Part 1 - Clean up isSystemOnlyAccessPermitted. v1

This is unspeakably nicer.
Attachment #665943 - Flags: review?(mrbkap) → review+
Attachment #665945 - Flags: review?(mrbkap) → review+
Attachment #665948 - Flags: review?(mrbkap) → review+
Comment on attachment 665947 [details] [diff] [review]
Part 3 - Warn about content access to |Components|. v1

Review of attachment 665947 [details] [diff] [review]:
-----------------------------------------------------------------

Wouldn't it be better to do this when we access the components property directly instead of when we get something off of the components object?
Attachment #665947 - Flags: review?(mrbkap)
(In reply to Blake Kaplan (:mrbkap) from comment #7)
> Wouldn't it be better to do this when we access the components property
> directly instead of when we get something off of the components object?

But how would we do that? That would require the Components object to become accessible via some kind of getter, right? I'm just kind of afraid of breaking something by defining it as a getter rather than a straight property...
Would it be possible to use one of SpiderMonkey's value getters (i.e. a getter without JSPROP_GETTER)? They are invisible from JS but run code on the C++ side. We'd only do it for content Components, clearly, reducing the possible damage.
Attachment #665947 - Attachment is obsolete: true
Attachment #666496 - Flags: review?(mrbkap)
Attachment #666495 - Flags: review?(mrbkap) → review+
Comment on attachment 666496 [details] [diff] [review]
Part 3 - Warn about content access to |Components|. v2

Review of attachment 666496 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/src/XPCComponents.cpp
@@ +4778,5 @@
> +        do_QueryInterface(nsJSUtils::GetStaticScriptGlobal(cx, obj));
> +    if (win) {
> +        nsCOMPtr<nsIDocument> doc =
> +            do_QueryInterface(win->GetExtantDocument());
> +        if (doc) {

Nit: no braces around single-line if in XPConnect.
Attachment #666496 - Flags: review?(mrbkap) → review+
Thanks Blake. Pushing to try:

https://tbpl.mozilla.org/?tree=Try&rev=6f6b441f1cbb
My assertion that obj was its global was too loose, since |obj| might be an outer window. Relaxed the check ever-so-slightly and repushed the borked runs:
https://tbpl.mozilla.org/?tree=Try&rev=f189c463ea57
Comment on attachment 666496 [details] [diff] [review]
Part 3 - Warn about content access to |Components|. v2

Review of attachment 666496 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/src/XPCComponents.cpp
@@ +4777,5 @@
> +    nsCOMPtr<nsPIDOMWindow> win =
> +        do_QueryInterface(nsJSUtils::GetStaticScriptGlobal(cx, obj));
> +    if (win) {
> +        nsCOMPtr<nsIDocument> doc =
> +            do_QueryInterface(win->GetExtantDocument());

For future reference, this could use GetExtantDoc()
Depends on: 797583
Comment on attachment 665945 [details] [diff] [review]
Part 2 - Introduce an explicit mechanism for determining if a script is from XBL. v1

This is needed for bug 810082.
Attachment #665945 - Flags: approval-mozilla-esr17+
Depends on: 864184
You need to log in before you can comment on or make changes to this bug.