If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Warn about Components removal and add Telemetry

RESOLVED FIXED in Firefox 18

Status

()

Core
XPConnect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

unspecified
mozilla18
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox18 fixed, firefox-esr1718+ fixed)

Details

Attachments

(5 attachments, 1 obsolete attachment)

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
Created attachment 665943 [details] [diff] [review]
Part 1 - Clean up isSystemOnlyAccessPermitted. v1

A lot of this stuff can be simplified now, and we can stop using the deprecated APIs.
Attachment #665943 - Flags: review?(mrbkap)
Created attachment 665945 [details] [diff] [review]
Part 2 - Introduce an explicit mechanism for determining if a script is from XBL. v1

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)
Created attachment 665947 [details] [diff] [review]
Part 3 - Warn about content access to |Components|. v1
Attachment #665947 - Flags: review?(mrbkap)
Created attachment 665948 [details] [diff] [review]
Part 4 - Telemetry. v1
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+

Updated

5 years ago
Attachment #665945 - Flags: review?(mrbkap) → review+

Updated

5 years ago
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.
Created attachment 666495 [details] [diff] [review]
Part 2.5 - Turn Components into a JS value getter for content scopes. v1
Attachment #666495 - Flags: review?(mrbkap)
Created attachment 666496 [details] [diff] [review]
Part 3 - Warn about content access to |Components|. v2
Attachment #665947 - Attachment is obsolete: true
Attachment #666496 - Flags: review?(mrbkap)

Updated

5 years ago
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
Green.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/00d03da9049a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/fb8bb9277152
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/43de19945cb1
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/aeda4978c97c
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/223f29a9e730
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()
https://hg.mozilla.org/mozilla-central/rev/00d03da9049a
https://hg.mozilla.org/mozilla-central/rev/fb8bb9277152
https://hg.mozilla.org/mozilla-central/rev/43de19945cb1
https://hg.mozilla.org/mozilla-central/rev/aeda4978c97c
https://hg.mozilla.org/mozilla-central/rev/223f29a9e730
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18

Updated

5 years ago
Depends on: 797583
Depends on: 799961
Blocks: 810082
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+

Updated

5 years ago
tracking-firefox-esr17: --- → 18+
https://hg.mozilla.org/releases/mozilla-esr17/rev/60e0608c2c2a
status-firefox18: --- → fixed
status-firefox-esr17: --- → fixed
Depends on: 864184
You need to log in before you can comment on or make changes to this bug.