Closed Bug 453649 Opened 16 years ago Closed 16 years ago

ecma/ExecutionContexts/10.2.2-1.js FAIL browser

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla1.9.1

People

(Reporter: bc, Assigned: enndeakin)

References

(
URL
)

Details

(Keywords: regression, testcase, verified1.9.1)

Attachments

(2 files)

fix
1.67 KB, patch
Details | Diff | Splinter Review
make globalStorage static
3.13 KB, patch
jst
: review+
jst
: superreview+
beltzner
: approval1.9.1+
Details | Diff | Splinter Review 
browser only, jit on or off

globalStorage == THIS[globalStorage] = false  FAILED! true

I believe this regressed in yesterday's nightly. I'll try to narrow down later today if no one beats me to it.
Flags: in-testsuite+
Flags: in-litmus-
Flags: blocking1.9.1?
PS. ecma/ExecutionContexts/10.2.2-2.js has the same problem.
Bob, did you get a chance to find a more narrow regression window?
Assignee: general → jorendorff
Both GetGlobalStorage calls go through the quick stub.

          |globalStorage| is innerWindow->GetGlobalStorage().
  |this["globalStorage"]| is outerWindow->GetGlobalStorage().
Shorter test case:

<!DOCTYPE HTML>
<script>
if (globalStorage != this["globalStorage"])
    alert('FAIL');
</script>

When no quick stub is present, the JS engine calls js_ComputeThis, which outerizes |this| among other things.  This is because XPC_WN_GetterSetter is a JSNative, not a JSPropertyOp like the quick stub getters/setters.  JS doesn't call js_ComputeThis for JSPropertyOps.

2 ways to fix:

- Treat this as a JSAPI bug and make JSPropertyOps do the whole ComputeThis song and dance.  Or, make the quick stubs outerize (same change, different location).

This would slow down all quick stub getters and setters, as all XPCWN objects' JSClasses have innerObject, outerObject, and thisObject.  I can measure how much.

- Make only Window quick stubs call ComputeThis.  Or just outerize (if that's safe).  ...Or don't put quick stubs on the window object at all, but then we lose on global |document|.

This approach is a bit subtler than it seems, since it depends on knowing which interfaces window classes implement.
Correction, document is handled specially in nsWindowSH::NewResolve.  We probably shouldn't be stubbing it.
document is handled very specially in nsDocumentSH::PostCreate. There isn't a need to stub it since we hand-stub it already.
Attached patch fix — — Splinter Review 
Blake suggested just making GetGlobalStorage forward to the outer window.  Most other nsGlobalWindow methods already forward to the appropriate half (inner or outer, depending on the feature).

So, sure, we can now call XPCOM methods on inner window objects, which is new, but that should be okay.

The fix also deletes the stub for window.document.  That change is not really related to the bug here.  It just happened to come up in comment 4 and I noticed the quick stub is dead code, shadowed by other magic.
Attachment #337943 - Flags: superreview?(jst)
Attachment #337943 - Flags: review?(jst)
I don't understand how the attached patch changes anything wrt globalStorage, really. The nsGlobalWindow::GetGlobalStorage() method simply returns a singleton object, whether that happens in the inner or outer window the result should be the same. Did I miss something here?
Er, jst, is gGlobalStorageList supposed to be 'static' on nsGlobalWindow? Right now it's per nsGlobalWindow (which is why Jason's patch works).
Wow, this goes way back, gGlobalStorageList was a *member* of nsGlobalWindow starting in Neil's first patch in bug 335540. Neil, any memory of this? Seems to me that there's no state and nothing in a storage list that ties it to a particular window, so it really should be a true global, as its name makes one think.
It seems like it should be yes. It was likely missed when first implemented as any data retrieved always comes fresh from the database, so whether it was global or not didn't matter. Now that we support the cookie-session permissions for this and are going to support private browsing, we should audit this more carefully and make sure the right data is being returned.
Neil, sounds like you'd be the best person to figure out the details here, wanna take this bug? If not, who do you think should?
Neil, I'm assigning this to you, and we need this fixed for 1.9.1. Please let me know if you won't be able to work on this.
Assignee: jorendorff → enndeakin
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
Target Milestone: --- → mozilla1.9.1
Filed bug 457914 for the window.document fix.
Attachment #337943 - Flags: superreview?(jst)
Attachment #337943 - Flags: review?(jst)

        Attachment #343606 -
        Flags: superreview?

        Attachment #343606 -
        Flags: review?

    
  

        Attachment #343606 -
        Flags: superreview?(jst)

        Attachment #343606 -
        Flags: superreview?

        Attachment #343606 -
        Flags: review?(jst)

        Attachment #343606 -
        Flags: review?

        Attachment #343606 -
        Flags: superreview?(jst)

        Attachment #343606 -
        Flags: superreview+

        Attachment #343606 -
        Flags: review?(jst)

        Attachment #343606 -
        Flags: review+

    
  
Component: JavaScript Engine → DOM
QA Contact: general → general

    
    

    
  
Does this need to be checked in?

    
    

    
  
mrbkap: yes.

    
    

    
  
Comment on attachment 343606 [details] [diff] [review]
make globalStorage static

We should get this in to fix the test.

        Attachment #343606 -
        Flags: approval1.9.1?

    
    

    
  
Comment on attachment 343606 [details] [diff] [review]
make globalStorage static

a191=beltzner, but this is a blocker so that's kinda redundant! :)

        Attachment #343606 -
        Flags: approval1.9.1? → approval1.9.1+

    
    

    
  
Neil, can you land this?

    
    

    
  
Neil landed this as
http://hg.mozilla.org/mozilla-central/rev/416fbf4fdef3

I then backed it out as
http://hg.mozilla.org/mozilla-central/rev/cd0a03d99535
on suspicion of causing failures in places sync tests (test_multiple_visits_around_sync.js and head_sync.js).

Those tests actually passed in the subsequent cycle (which didn't include the backout), but I'm leaving this backed out for now, because the tests aren't known to be sporadic failures, and it seems possible that this checkin makes them sporadically fail.

For reference, here's the log with the failures, and then the log with them subsequently passing (not including the backout)
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1228495584.1228498256.22059.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1228498362.1228500257.27636.gz

    
    

    
  
(In reply to comment #22)
> because the tests aren't
> known to be sporadic failures

OK -- it turns out they were known to be sporadic failures, though the first one (test_multiple_visits_around_sync.js) wasn't noted on the orange compendium.  (head_sync.js was noted there, though -- I thought I'd searched for it, but I guess not - my bad)

I've now updated the compendium to include test_multiple_visits_around_sync.js, and I added a line for head_sync.js to reflect today's test-failure.

Neil, sorry about the backout -- can you re-land later on?

    
  
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Whiteboard: [needs 1.9.1 landing]

    
    

    
  
Neil, can you land this one?

    
  
Keywords: fixed1.9.1
Whiteboard: [needs 1.9.1 landing]

    
  
Blocks: 479211

    
    

    
  
pushed: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/dced1fb507d7

Seeing as there hasn't been any discussions about this bug for 2 months and it's been in mochitest, I'm assuming there aren't any residual issues. I'm moving this to verified as a result. If anyone has any qualms, feel free to bring them up.
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1 → verified1.9.1
Component: DOM → DOM: Core & HTML

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: