298730 - ASSERTION: unexpected target property value: '!JSVAL_IS_PRIMITIVE(*vp)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bob Clary [:bc] (inactive)]

I first started seeing this assertion on 6/22 on the 1.0.5 branch only in windows.

###!!! ASSERTION: unexpected target property value: '!JSVAL_IS_PRIMITIVE(*vp)',
file ./mozilla/caps/src/nsScriptSecurityManager.cpp, line 458

nsScriptSecurityManager::CheckObjectAccess(JSContext * 0x00f0eec8, JSObject *
0x02d4abc8, long 47490132, JSAccessMode JSACC_WATCH, long * 0x0012f144) line 458
+ 46 bytes
js_CheckAccess(JSContext * 0x00f0eec8, JSObject * 0x02d4abc8, long 16345400, int
3, long * 0x0012f144, unsigned int * 0x0012f140) line 3413 + 50 bytes
js_Interpret(JSContext * 0x00f0eec8, long * 0x0012f260) line 3954 + 48 bytes
js_Execute(JSContext * 0x00f0eec8, JSObject * 0x02d4a220, JSScript * 0x00fbc8a8,
JSStackFrame * 0x00000000, unsigned int 0, long * 0x0012f38c) line 1173 + 13 bytes
JS_ExecuteScript(JSContext * 0x00f0eec8, JSObject * 0x02d4a220, JSScript *
0x00fbc8a8, long * 0x0012f38c) line 3540 + 25 bytes
mozJSComponentLoader::GlobalForLocation(const char * 0x00fb0d98, nsIFile *
0x02c9ada8) line 1120 + 27 bytes
mozJSComponentLoader::ModuleForLocation(const char * 0x00fb0d98, nsIFile *
0x02c9ada8) line 913 + 19 bytes
mozJSComponentLoader::AttemptRegistration(nsIFile * 0x02c9ada8, int 0) line 749
+ 24 bytes
mozJSComponentLoader::AutoRegisterComponent(mozJSComponentLoader * const
0x00fb0e70, int 0, nsIFile * 0x02c9ada8, int * 0x0012f680) line 675 + 14 bytes
mozJSComponentLoader::RegisterComponentsInDir(int 0, nsIFile * 0x003e8380) line
583 + 24 bytes
mozJSComponentLoader::AutoRegisterComponents(mozJSComponentLoader * const
0x00fb0e70, int 0, nsIFile * 0x003e8380) line 540
nsComponentManagerImpl::AutoRegisterNonNativeComponents(nsIFile * 0x003e8380)
line 3280 + 47 bytes
nsComponentManagerImpl::AutoRegisterImpl(int 0, nsIFile * 0x00000000, int 1)
line 3250 + 20 bytes
nsComponentManagerImpl::AutoRegister(nsComponentManagerImpl * const 0x003eeca8,
nsIFile * 0x00000000) line 3422 + 19 bytes
NS_InitXPCOM2(nsIServiceManager * * 0x0012fb04, nsIFile * 0x003e7740,
nsIDirectoryServiceProvider * 0x0012ff40) line 603
ScopedXPCOMStartup::Initialize() line 795 + 29 bytes
xre_main(int 1, char * * 0x003e7678, const nsXREAppData * 0x0041e01c kAppData)
line 1783 + 11 bytes
main(int 1, char * * 0x003e7678) line 58 + 18 bytes

id 16345400 (0x00F96938) is val 47490132 (0x02D4A454) = string (0x02D4A450)
"operation"

Johnny says that js_CheckAccess should only be called for a limited number of
properties like __proto__ etc and not for things like this.

Comment 1

[Bob Clary [:bc] (inactive)]

To reproduce, just start the browser.

Comment 2

[Daniel Veditz [:dveditz]]

likely a regression from bug 296397

Comment 3

[Jay Patel [:jay]]

Brendan says set XPCOM_DEBUG_BREAK=warn, we aren't going to fix this for 1.0.5

Comment 4

[Daniel Veditz [:dveditz]]

How many times does the assertion fire? If it's firing a lot then it might
indicate we're checking a lot more than we intend to--if so the patch for bug
296397 might not be doing the right thing.

Otherwise we can just fix an innocuous warning assertion on the trunk. Basically
it comes down to the question "In the future 'assertions are fatal' world, would
this one be left as an assertion or changed to a warning?" If it's just a
warning we don't need to rush a fix for the stable branch.

Comment 5

[Brendan Eich [:brendan]]

Re: comment 2: Actually this is not a regression from bug 296397 -- jst just
removed the assertion on the trunk.  I can do the same on the branch if someone
wants to approve that.

/be

Comment 6

[Johnny Stenback (:jst)]

Brendan, I thought we fixed the JS engine also to always get the value of the
property we're checking for before doing the check. That should make us not hit
this assertion, or was that done only for __parent__ and __proto__ or something?

Comment 7

[Bob Clary [:bc] (inactive)]

(In reply to comment #4)
> How many times does the assertion fire? 

24 times on startup and for the occasional page. You can see the most recent log
at <http://test.bclary.com/results/2005-06-30-firefox-1.0.x-dbg-01.log>

Comment 8

[Brendan Eich [:brendan]]

(In reply to comment #6)
> Brendan, I thought we fixed the JS engine also to always get the value of the
> property we're checking for before doing the check.

No, we didn't need or want that cost -- getters are not idempotent, or even
necessarily cheap, etc.  In this case, some JS object initialisers are using get
and set syntax to declare getters and setters, so there is no such property
before the first (usually getter) is declared.

> That should make us not hit
> this assertion, or was that done only for __parent__ and __proto__ or something?

Only for proto and parent, based on JSACC_PROTO/PARENT, and using the directly
addressed object, not the prototype object in which the property was found, cuz
__proto__ and __parent__ are magic.

Again, you (how soon we forget ;-) removed the bogo-assertion in trunk rev 1.261
of nsScriptSecurityManager.cpp.  We could put that change on the branch.  All I
need is the word from jay.

/be

Comment 9

[Brendan Eich [:brendan]]

Attachment: jst's trunk patch to rid us of this irritation

Here's the diff.

/be

Comment 10

[Jay Patel [:jay]]

Comment on attachment 187901 [details] [diff] [review]
jst's trunk patch to rid us of this irritation

Yes, let's get that patch in. a=jay

Comment 11

[Brendan Eich [:brendan]]

Fixed on branches.

/be

Comment 12

[Jay Patel [:jay]]

Removing keywords until we know this made it onto the suite.

Brendan:  I don't see a checkin for this on the 1.7 branch, can you take a look
and get that in asap?  If it's already in, please return the fixed keyword.

Comment 13

[Brendan Eich [:brendan]]

Sorry, forgot to hit the MOZILLA_1_7_BRANCH -- done.

/be