Bug 41864

Summary:

[V8] Navigation policy doesn't play nicely with pepper plugins

Product:

WebKit

Reporter:

Adam Barth <abarth>

Component:

New Bugs

Assignee:

Adam Barth <abarth>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

fishd, jam

Priority:

Version:

528+ (Nightly build)

Hardware:

Other

OS:

OS X 10.5

Attachments:

Description	Flags
Patch	none
Updated patch	none
Patch	none
Patch	fishd: review+, abarth: commit-queue-

Adam Barth

Reported 2010-07-08 09:19:53 PDT

[V8] Navigation policy doesn't play nicely with pepper plugins

Attachments
Patch (1.67 KB, patch) 2010-07-08 09:21 PDT, Adam Barth	no flags	Details Formatted Diff Diff
Updated patch (1.58 KB, patch) 2010-07-08 11:05 PDT, John Abd-El-Malek	no flags	Details Formatted Diff Diff
Patch (2.35 KB, patch) 2010-07-08 13:03 PDT, John Abd-El-Malek	no flags	Details Formatted Diff Diff
Patch (2.75 KB, patch) 2010-07-08 15:53 PDT, John Abd-El-Malek	fishd: review+ abarth: commit-queue-	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Adam Barth

Comment 1 2010-07-08 09:21:54 PDT

Created attachment 60898 [details] Patch

John Abd-El-Malek

Comment 2 2010-07-08 11:05:08 PDT

Created attachment 60913 [details] Updated patch

Adam Barth

Comment 3 2010-07-08 12:14:40 PDT

Comment on attachment 60913 [details] Updated patch This patch does not have a ChangeLog. V8Utilities:103 + Frame* callingFrameForNavigation() { This isn't the right name for this function. The calling frame isn't changing because we're interested in navigation. Maybe the name "activeFrameForNavigation" is better? It's really the same thing we do for the canAccess check, so it might be worth sharing there too. Maybe principalFrame (to steal some terminology from Mozilla)?

Darin Fisher (:fishd, Google)

Comment 4 2010-07-08 12:48:22 PDT

I was struggling to suggest a name too. I like principalFrame since it helps specify how we intend to use the frame.

Darin Fisher (:fishd, Google)

Comment 5 2010-07-08 12:49:15 PDT

Another choice is callingOrEnteredFrame. It seems common in WebKit to use function names like that. It doesn't add any new terms to understand and just states what the function does.

John Abd-El-Malek

Comment 6 2010-07-08 13:03:20 PDT

Created attachment 60937 [details] Patch

John Abd-El-Malek

Comment 7 2010-07-08 13:03:59 PDT

callingOrEnteredFrame sounds good to me.

Adam Barth

Comment 8 2010-07-08 15:19:51 PDT

Comment on attachment 60937 [details] Patch - if (!callingFrame) - return; You removed this null check. Retrieving the frame can fail if the entered context is associated with an inactive document. In that case, this patch introduces a crash.

Adam Barth

Comment 9 2010-07-08 15:20:20 PDT

Comment on attachment 60937 [details] Patch WebCore/bindings/v8/V8Utilities.cpp:119 + Frame* callingFrame = callingOrEnteredFrame(); Also, "callingFrame" is not an accurate name anymore.

John Abd-El-Malek

Comment 10 2010-07-08 15:53:41 PDT

Created attachment 60975 [details] Patch

Adam Barth

Comment 11 2010-07-08 16:28:32 PDT

Thanks John.

Adam Barth

Comment 12 2010-07-08 16:42:46 PDT

Comment on attachment 60975 [details] Patch This will actually fail to land in the commit-queue because the ChangeLog says OOPS. I can land it manually for John.

John Abd-El-Malek

Comment 13 2010-07-08 16:43:55 PDT

Thanks for the initial pointer and review. I can take care of landing it.

John Abd-El-Malek

Comment 14 2010-07-08 16:59:52 PDT

Committed r62868: <http://trac.webkit.org/changeset/62868>

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