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.

Created attachment 337943[details][diff][review]
fix
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.

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?

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.

(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?

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

CC: adesai

Keywords: fixed1.9.1 → verified1.9.1

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