Bug 25634 - Need a way to get the Frame for the code at the top of the JS stack
Summary: Need a way to get the Frame for the code at the top of the JS stack
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-05-07 17:26 PDT by Aaron Boodman
Modified: 2009-05-11 14:40 PDT (History)
6 users (show)

See Also:


Attachments
Patch (6.17 KB, patch)
2009-05-07 17:31 PDT, Aaron Boodman
no flags Details | Formatted Diff | Diff
Fix formatting and other issues in ChangeLog (6.14 KB, patch)
2009-05-07 17:33 PDT, Aaron Boodman
fishd: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Aaron Boodman 2009-05-07 17:26:17 PDT
Right now, in the Chromium port, there is ScriptController::retrieveActiveFrame(), which retrieves the WebCore Frame corresponding to the JS code at the bottom of the execution stack. We sometimes also need to get the Frame corresponding to the code at the top of execution stack, which can be different.
Comment 1 Aaron Boodman 2009-05-07 17:31:53 PDT
Created attachment 30119 [details]
Patch
Comment 2 Aaron Boodman 2009-05-07 17:33:57 PDT
Created attachment 30120 [details]
Fix formatting and other issues in ChangeLog
Comment 3 Adam Barth 2009-05-08 13:41:38 PDT
We almost always want the bottom of the stack.  Why do we want to the top of the stack in these cases?
Comment 4 Aaron Boodman 2009-05-08 13:50:50 PDT
(In reply to comment #3)
> We almost always want the bottom of the stack.  Why do we want to the top of
> the stack in these cases?

You can check out the related Chromium change. When an Extension API is called, we want to know which frame invoked it so that we can be more intelligent with the behavior of APIs based on what RenderView they came from.

For example: we have a createWindow() API. We want the new window be positioned relative to the window the API call originated from. In order to know which window the call originated from, we need to know which renderview it originated from. Since JavaScript calls can traverse frame boundaries, the bottom of the stack can give the wrong answer.

An easy example of this is the inspector. You can call chromium.windows.create() from the inspector. We want the new window to be positioned relative to the window you're inspecting, not the inspector window.

But there are other examples.
Comment 5 Aaron Boodman 2009-05-08 13:53:21 PDT
Here's the related Chromium change:
http://codereview.chromium.org/113085
Comment 6 Adam Barth 2009-05-08 14:16:36 PDT
(In reply to comment #4)
> You can check out the related Chromium change. When an Extension API is called,
> we want to know which frame invoked it so that we can be more intelligent with
> the behavior of APIs based on what RenderView they came from.

I don't understand why the extension case differs from the general web case.

> An easy example of this is the inspector. You can call
> chromium.windows.create() from the inspector. We want the new window to be
> positioned relative to the window you're inspecting, not the inspector window.

The context for evaluating scripts in the web inspect is all screwed up.  I don't think we should bend the web platform to fix the needs of the inspector.

The thing is, a bunch of these changes violate the HTML 5 spec.  For example, the change to postMessage means that the source would be determined via dynamic scope instead of via lexical scope, as required by the spec.

Similarly, in DOMWindowOpen and WindowSetLocation, you're now applying the frame navigation policy (shouldAllowNavigation) based on dynamic scope instead of lexical scope, in violation of the spec and the behavior of other browsers.

The changes to HTMLDocumentWrite and HTMLDocumentWriteln change our behavior when using these APIs.  Have you tested this to see whether this makes us more or less compatible with other browsers?  I suspect it makes use less compatible.

Basically, I don't think its a good idea to subtly change a bunch of behavior for "consistency" without considering the compatibility and compliance issues.
Comment 7 Adam Barth 2009-05-08 15:05:56 PDT
I spoke with Aaron on #webkit.  I mistakenly thought this patch was changing our behavior.  I think we just have lots of bugs in this department.

We should change the V8 bindings to match the JSC bindings in where we use the lexical or dynamic global object, but we can do that in a separate patch.
Comment 8 Aaron Boodman 2009-05-08 15:12:20 PDT
Adding a larger group of Chromium reviewers, hoping that somebody can give this an r+.

To clarify, this change does not change behavior. It's just adding a new method and renaming an existing one.
Comment 9 Adam Barth 2009-05-08 15:14:07 PDT
Yes.  Please disregard my comment #6.  That's a bug for another day.  :)
Comment 10 Dimitri Glazkov (Google) 2009-05-11 14:32:02 PDT
Landed as http://trac.webkit.org/changeset/43512.
Comment 11 Adam Barth 2009-05-11 14:40:50 PDT
Follow up filed as https://bugs.webkit.org/show_bug.cgi?id=25706