484040 - Access to document seems to fall off trace

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[Boris Zbarsky [:bzbarsky]]

I was trying the following loop with bent's patch to trace quickstubs:

      for (var i = 0; i < 100000; ++i) {
        document.getElementById("a"+i);
      }

using as my test page the HTML5 single-page spec with all scripts stripped out.

We seem to bail off trace on access to the "document" property, with TM claiming that there's a getter.  Given the code in nsDocumentSH::PostCreate, this is rather odd.

Comment 1

[Boris Zbarsky [:bzbarsky]]

OK, I hit this again (it was the only thing preventing a DOM testcase from being jitted), so I did some digging.

We bail at this stack:

#0  TraceRecorder::isValidSlot (this=0x19d4a870, scope=0x19d41e50, sprop=0x1437a70) at /Users/bzbarsky/mozilla/vanilla/mozilla/js/src/jstracer.cpp:1950
#1  0x003a8745 in TraceRecorder::test_property_cache_direct_slot (this=0x19d4a870, obj=0x16abc600, obj_ins=0x7ff098, slot=@0xbfffb754) at /Users/bzbarsky/mozilla/vanilla/mozilla/js/src/jstracer.cpp:6184
#2  0x003af453 in TraceRecorder::name (this=0x19d4a870, vp=@0xbfffb79c) at /Users/bzbarsky/mozilla/vanilla/mozilla/js/src/jstracer.cpp:8321
#3  0x003af73e in TraceRecorder::record_JSOP_NAME (this=0x19d4a870) at /Users/bzbarsky/mozilla/vanilla/mozilla/js/src/jstracer.cpp:8554
#4  0x003b3e59 in TraceRecorder::monitorRecording (cx=0x12e6e00, tr=0x19d4a870, op=JSOP_NAME) at jsopcode.tbl:180
#5  0x002c7175 in js_Interpret (cx=0x12e6e00) at /Users/bzbarsky/mozilla/vanilla/mozilla/js/src/jsinterp.cpp:3015

In isValidSlot, we have:

(gdb) p *sprop
$3 = {
  id = 16750412, 
  getter = 0xb2a0d2 <XPC_WN_Helper_GetProperty>, 
  setter = 0xb29fe6 <XPC_WN_Helper_SetProperty>, 
  slot = 50, 
  attrs = 3 '\003', 
  flags = 0 '\0', 
  shortid = 0, 
  parent = 0x1437a50, 
  kids = 0x146b090, 
  shape = 12638
}

In test_property_cache_direct_slot we have:

(gdb) jsclass obj
$6 = {
  name = 0x1bef7e90 "Window", 
  flags = 860429, 
  addProperty = 0xb2a2aa <XPC_WN_Helper_AddProperty>, 
  delProperty = 0xb2a1be <XPC_WN_Helper_DelProperty>, 
  getProperty = 0xb2a0d2 <XPC_WN_Helper_GetProperty>, 
  setProperty = 0xb29fe6 <XPC_WN_Helper_SetProperty>, 
  enumerate = 0x270398 <JS_EnumerateStub>, 
  resolve = 0xb2c0f6 <XPC_WN_Helper_NewResolve>, 
  convert = 0xb273c8 <XPC_WN_Shared_Convert>, 
  finalize = 0xb29e98 <XPC_WN_Helper_Finalize>, 
  getObjectOps = 0xb282da <XPC_WN_GetObjectOpsNoCall(JSContext*, JSClass*)>, 
  checkAccess = 0xb29cac <XPC_WN_Helper_CheckAccess>, 
  call = 0, 
  construct = 0, 
  xdrObject = 0, 
  hasInstance = 0, 
  mark = 0xb27c1c <XPC_WN_Shared_Trace>, 
  reserveSlots = 0
}

Specifically, |obj| is an inner window:

(gdb) p ((XPCWrappedNative*)(obj->fslots[2] &~1))->mIdentity  
$21 = (nsGlobalWindow *) 0x19d44650
(gdb) p $21->mIsInnerWindow
$22 = 1 '\001'
(gdb) p $21->mOuterWindow
$23 = (nsGlobalWindow *) 0x1bef71b0
(gdb) p $21->mInnerWindow
$24 = (Cannot access memory at address 0x0

But we definitely set the property on the inner window in nsDocumentSH::PostCreate.  I just double-checked that.

So I'm still not sure what's going on here.

Comment 2

[Jeff Walden [:Waldo]]

I have a guess, will report back after testing.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Oh, of course.  The JS_DefineUCProperty call in question passes a null getter and setter, so the class ops get used.

I just looked through the Get/Set hooks for nsWindowSH, and they don't need to run for document on the _inner_ window.  So I think we could pass the stub getter/setter here and just accessing |document| would work without falling off trace.  Accessing |window.document| would still fall off trace.

Worth doing this for 1.9.1?  Or are we landing general tracing of getters/setters there so that we can just stop worrying about this bug?

Comment 4

[Brendan Eich [:brendan]]

Heh, just mid-aired with bz. This is an easy 1.9.1 fix, akin to the same deal for undefined that dmandelin did the other week to win back lost browser vs. shell performance.

/be

Comment 5

[Brendan Eich [:brendan]]

Any more JS_Define*Property calls like that?

/be

Comment 6

[Jeff Walden [:Waldo]]

I'll take a look (and yes, that was the hunch, doing a rebuild now just to be entirely sure) and see if there are.

Comment 7

[Andreas Gal :gal]

Its always faster if we don't have to call the hook to get the value once it was resolved.

Comment 8

[Johnny Stenback (:jst)]

This looks trivial to fix with a potentially high gain, blocking.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Talking to jst today... it would be good to define the property on the outer window in addition to the inner one.  Then reget the value when we get a new inner (probably by removing the property and then resetting it if the inner already has such a property; otherwise waiting for the postcreate hook to set it up).

Comment 10

[Jeff Walden [:Waldo]]

(In reply to comment #6)
> I'll take a look (and yes, that was the hunch, doing a rebuild now just to be
> entirely sure) and see if there are.

Yeah, there are a few dozen or so of them, not all capable of being trivially modified (e.g. some NPAPI-related uses).  Spot-fix may be easy (although comment 9 hints at additional complexity with which I may not be completely familiar), addressing completely is likely tedious both to patch and to review -- punting to jst as I recall he expressed willingness to find someone else to do this...

Comment 11

[Johnny Stenback (:jst)]

Attachment: Fix.

I believe this changes all the callers of JS_Define* that we can safely change over to passing JS_PropertyStub as the getter and/or setter. This also removes class get/setproperty hooks from the global scope polluter so that we can use the stubs there too, the only reason for it existing was to do security checks we no longer need, and to report to the console, which we can do from the resolve hook just as well (fewer commonly duplicate errors on the console, which is probably a good thing anyways).

Comment 12

[Boris Zbarsky [:bzbarsky]]

Do you want to do the outer window thing in a separate bug then (and remove "window." from the summary off this bug)?

Comment 13

[Peter Van der Beken [:peterv]]

Comment on attachment 376144 [details] [diff] [review]
Fix.

I think you can remove GlobalScopePolluterGetProperty too, looks unused.

Comment 14

[Johnny Stenback (:jst)]

Attachment: document only fix.

This patch is a small part of the above attachment, and only changes how access to document behaves. Much safer of course, but catches far fewer cases.

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 376355 [details] [diff] [review]
document only fix.

r+sr=mrbkap

Comment 16

[Johnny Stenback (:jst)]

Comment on attachment 376144 [details] [diff] [review]
Fix.

Clearing review requests, we'll do the rest of this in a new bug.

Comment 17

[Boris Zbarsky [:bzbarsky]]

That sounds fine; I'd be happy to review the big patch (I was partway through anyway).  Please do get a bug filed on the outer window, though.

Comment 18

[Johnny Stenback (:jst)]

Fixed on trunk and branch. bug 492649 filed as a followup.

http://hg.mozilla.org/mozilla-central/rev/f4d5b61bb7a7
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/1607a94fbda1

Comment 19

[Boris Zbarsky [:bzbarsky]]

We need a second followup too, for window.document.  Right?

Comment 20

[Johnny Stenback (:jst)]

bz, if we're going to go down that route I'd say we take the whole thing. I pushed an updated patch to the try server a bit earlier, but the try servers seem far from happy (for reasons beyond my patch). I'll probably try again once the dust settles a bit, and assuming it passes I'd say we just take the whole thing. Thoughts?

Comment 21

[Boris Zbarsky [:bzbarsky]]

I'd be pretty happy with that, yes.

Comment 22

[Johnny Stenback (:jst)]

Oops, forgot to mark this fixed.