471560 - "ASSERTION: Must be able to access currentInner" with Google custom search widget

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Jesse Ruderman]

Attachment: partially-reduced testcase

###!!! ASSERTION: Must be able to access currentInner!: 'nsContentUtils::CanCallerAccess(currentInner)', file /Users/jruderman/central/dom/src/base/nsDOMClassInfo.cpp, line 5256

This assertion was added in bug 467210.

Security-sensitive for now because this is an assertion failure in code related to security.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

I think I saw this assertion also when running mochitest (or chrome or browser-chrome).

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #1)
> I think I saw this assertion also when running mochitest (or chrome or
> browser-chrome).

NS_ASSERTION is so useless because it's impossible to notice if you add an assertion during mochitests.

So, I think this might be OK actually. What's happening here is that we're resolving the (allAccess) location property on a foreign frame. This, in turn, defines the Location constructor on that foreign frame. In the process of creating the Location constructor, we create it with the bugzilla script still on the call stack, hitting the assertion.

This is okay, though, because the calling script can't access the constructor, and we do another check in BaseStubConstructor. I think we should be able to remove the assertion or push the window's context in nsWindowSH::GlobalResolve.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #2) 
> NS_ASSERTION is so useless because it's impossible to notice if you add an
> assertion during mochitests.
I pretty often log the output of mochitest and grep assertions.

Comment 4

[Jeff Walden [:Waldo]]

NS_ABORT_IF_FALSE is the new NS_ASSERTION.  If you want to assert something, use that -- it can't be ignored.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

pretty off topic, but running tests doesn't assert too much nowadays,
especially chrome and browser-chrome. There are few assertions which shouldn't
be assertions but warnings and other things we should just fix.
IIRC the assertion count when running the whole mochitest has dropped to 1/3 what
it used to be still some time ago.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I'm able to reproduce this type of assertion while running a crashtest within widgets/src/cocoa. See bug 471978.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: So, comme ça? [Checkin: Comment 9 & 12]

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 355486 [details] [diff] [review]
So, comme ça?
[Checkin: Comment 9 & 12]

Works for me!

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/mozilla-central/rev/3153bca51559

Comment 10

[Daniel Veditz [:dveditz]]

Not a security bug given the fix and comment 2, but if still wanted on 1.9.1 a motivated developer needs to request approval on the patch.

Comment 11

[Daniel Veditz [:dveditz]]

Comment on attachment 355486 [details] [diff] [review]
So, comme ça?
[Checkin: Comment 9 & 12]

Approved for 1.9.1.4, a=dveditz for release-drivers

Comment 12

[Serge Gautherie (:sgautherie)]

Comment on attachment 355486 [details] [diff] [review]
So, comme ça?
[Checkin: Comment 9 & 12]


http://hg.mozilla.org/releases/mozilla-1.9.1/rev/a987674af997

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified on 1.9.1 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1.5pre) Gecko/20091008 Shiretoko/3.5.5pre ID:20091008213747