Closed Bug 1042436 Opened 5 years ago Closed 5 years ago

Add more visible warnings when property access silently fails due to XrayToJS or Opaque Xrays

Categories

(Core :: XPConnect, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34
Tracking Status
firefox32 --- fixed
firefox33 --- fixed
firefox34 --- fixed

People

(Reporter: bholley, Assigned: bholley)

References

Details

(Keywords: addon-compat)

Attachments

(3 files, 1 obsolete file)

Bug 856067 and bug 987111 were major milestones in securing interaction between content and privileged JS. The downside, of course, is that the filtering behavior may cause addons to fail somewhat inexplicably.

There's currently a mechanism to dump to the console in debug builds when this happens, but I've come to the conclusion that we should report something to the browser console as well.
Sorry I've been slow on this - Some other Mozilla stuff came up, and I've also been distracted with a family medical emergency. I'll try to take a crack at this today.
(In reply to Bobby Holley (:bholley) from comment #1)
> Sorry I've been slow on this - Some other Mozilla stuff came up, and I've
> also been distracted with a family medical emergency. I'll try to take a
> crack at this today.

I hope everything is alright, and thanks for the feedback.
Gabor for review, smaug to make sure I'm using the console correctly, and
jorge to confirm that this is the right thing for addon devs.
Attachment #8468214 - Flags: superreview?(bugs)
Attachment #8468214 - Flags: review?(gkrizsanits)
Attachment #8468214 - Flags: feedback?(jorge)
Attachment #8468213 - Flags: review?(gkrizsanits) → review+
Comment on attachment 8468214 [details] [diff] [review]
Part 2 - Warn once to the console when XrayWrappers deny access to an object. v1

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

r=me with a few minor changes:

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +583,5 @@
> +
> +    // The browser console warning is only emitted for the first violation,
> +    // whereas the (debug-only) NS_WARNING is emitted for each violation.
> +#ifndef DEBUG
> +    if (alreadyWarnedOnce)

How about adding a pref flag for non-debug mode here that can turn on the debug behavior?

@@ +602,5 @@
> +
> +    // If this isn't the first warning on this topic for this global, we've
> +    // already bailed out in opt builds. Now that the NS_WARNING is done, bail
> +    // out in debug builds as well.
> +    if (alreadyWarnedOnce)

Shouldn't the debug build bail out after NS_WARNING always? It seems to me that in debug build for the first error we are going to warn twice, and the second one with the message "only the first ... will be reported" which is not true...

@@ +609,5 @@
> +    //
> +    // Log a message to the console service.
> +    //
> +
> +    // Grab the pieces.

Can we do this whole console code bellow in some common place, like nsContentUtils or xpcprivate? I find it useful as a utility function, and does not look like some code I wanted to write myself / duplicate very often...
Attachment #8468214 - Flags: review?(gkrizsanits) → review+
nsContentUtils has almost similar code. Almost, but not exactly, so reusing that here wouldn't quite work.

gabor, NS_WARNING goes to the terminal, so I'm not sure why that reporting is relevant here.
Comment on attachment 8468214 [details] [diff] [review]
Part 2 - Warn once to the console when XrayWrappers deny access to an object. v1

I think the comment about XRayWrapper denying access isn't clear enough.
The comment should tell why the access was denied, not what denied the access.
Attachment #8468214 - Flags: superreview?(bugs) → superreview-
(In reply to Olli Pettay [:smaug] from comment #6)
> gabor, NS_WARNING goes to the terminal, so I'm not sure why that reporting
> is relevant here.

Ah right, I've got a bit confused there... But wouldn't the console warning show on the
terminal as well? Anyway, it does not matter, even if does we still want to keep both,
so just ignore my comment.
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #8)
> Ah right, I've got a bit confused there... But wouldn't the console warning
> show on the
> terminal as well?

I don't believe that it does.
Attachment #8468214 - Attachment is obsolete: true
Attachment #8468214 - Flags: feedback?(jorge)
Attachment #8468686 - Flags: superreview?(bugs)
Attachment #8468686 - Flags: review+
Attachment #8468684 - Attachment description: Part 1.5 - Lowercase the "reason" param to SilentFailure. v1 r=me → Part 2 - Lowercase the "reason" param to SilentFailure. v1 r=me
Attachment #8468686 - Flags: superreview?(bugs) → superreview+
This should be in tomorrow's Nightly. Jorge, can you play around with it tomorrow and let me know if the behavior is satisfactory? Assuming it is, I'd like to get this backported as soon as possible.
Flags: needinfo?(jorge)
The message looks good, thanks!
Flags: needinfo?(jorge)
Comment on attachment 8468686 [details] [diff] [review]
Part 3 - Warn once to the console when XrayWrappers deny access to an object. v3 r=gabor

Great! Flagging for uplift.

Bug 987111 is now on beta. We had some addon-compat concerns, and thankfully we have received scant reports of any breakage, which is great. Regardless though, Jorge would like to be able to point developers to the browser console for information about whether the new security wrappers interfered with a given addon. This information will drastically reduce confusion about the cause of any breakage and the time needed to track down what line of code to fix.

Approval Request Comment
[Feature/regressing bug #]: bug 987111, in a sense. 
[User impact if declined]: Potential pain for addon developers adapting to bug 987111.
[Describe test coverage new/current, TBPL]: Landed on m-c. Automated test coverage.
[Risks and why]: Low risk 
[String/UUID change made/needed]: The message is non-localized, so no string changes were made.
Attachment #8468686 - Flags: approval-mozilla-beta?
Attachment #8468686 - Flags: approval-mozilla-aurora?
Attachment #8468686 - Flags: approval-mozilla-beta?
Attachment #8468686 - Flags: approval-mozilla-beta+
Attachment #8468686 - Flags: approval-mozilla-aurora?
Attachment #8468686 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.