Bug 42827 – Use the js-execute-active(entered) Frame to check the user gesture of page instead of checking the top frame

• Component: Frames
• Status: RESOLVED FIXED

[Johnny(Jianning) Ding]

Created attachment 62300 [details]
test case

On chromium with enabling popup blocker, http://watchthesimpsons.com/ bypassed the popup blocker [firefox does block it]

The attachment is the test case to reproduce this issue.

The following is my investigation.

The window.open is called in the iframe without user-initiated.

On Safari, it uses WebCore::Setting::m_javaScriptCanOpenWindowsAutomatically flag to do the popup blocker, when enabling popup blocker, m_javaScriptCanOpenWindowsAutomatically is set to true, so DOMWindow::allowPopUp returns false to deny to open the window. So this issue can not be reproduced on Safari.
On Chromium, the flag WebCore::Setting::m_javaScriptCanOpenWindowsAutomatically is always true, the request of opening window is always passed to browser process to do the popup check. When enabling chromium's popup blocker, only user-initiated open request can be allowed.

In this issue, the window.open was called in the iframe without user-initiated. but when calling WebFrame::isProcessingUserGesture in RenderView::createView, in the function FrameLoader::isProcessingUserGesture, it checked top frame's user gesture status, since there was no script executed in top frame, ScriptController::processingUserGesture assumed it's user-initiated (line 166-167). Then chromium treated the opening window request is legal and granted the opening.
I think the current behavior in the frameloader is wrong, we should check the js-execute-active frame (entered frame in V8) instead of the top frame since the script execution can be initiated in any frame not only the top frame. Please refer to the right implementation of the WebCore::processingUserGesture (both JS and V8 version)

The issue does not reproduce on Chromium v5 stable and popup is blocked, but reproducible on the latest Chrome trunk. After digging in the WebKit trunk, I found before r62380 (http://trac.webkit.org/changeset/62380), the function ScriptController::processingUserGesture did only check the active frame's user gesture status not current frame, so even we call the function to check the top frame's user gesture status, it still can returned the right status. But just because the function tested if there are any Frame on the stack where JS is executing, it caused bug https://bugs.webkit.org/show_bug.cgi?id=41511, that's why Yury changed it in r62380.


My solution is adding a function in ScriptController to get the active/entered frame, so the function FrameLoader::isProcessingUserGesture can correctly test the user gesture status of the page.

Any comments?

@Darin, your are the author of FrameLoader(according to http://trac.webkit.org/changeset/17652/), would you please give your comments?

[Johnny(Jianning) Ding]

related chromium bug: http://crbug.com/49745

[Adam Barth]

Maybe a better solution is to make the "check for user gesture" function on ScriptController static and have it figure out what frame it's supposed to ask.

[Johnny(Jianning) Ding]

(In reply to comment #2)
> Maybe a better solution is to make the "check for user gesture" function on ScriptController static and have it figure out what frame it's supposed to ask.

the ScriptController::processingUserGesture before r62380 did find a active frame to check its status of user gesture, but looked like some tests needed a function to know whether a single frame was processing user gesture or not without caring about the other frames according to bug https://bugs.webkit.org/show_bug.cgi?id=41511.

[Johnny(Jianning) Ding]

Created attachment 62366 [details]
patch v1, adding a getActiveFrame method to get current active JS execute frame

[WebKit Review Bot]

Attachment 62366 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/bindings/js/ScriptController.cpp:306:  Use 0 instead of NULL.  [readability/null] [5]
WebCore/loader/FrameLoader.cpp:1132:  Tab found; better to use spaces  [whitespace/tab] [1]
Total errors found: 2 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Johnny(Jianning) Ding]

Created attachment 62371 [details]
patch v1, adding a getActiveFrame method to get current active JS execute frame

fixed style errors

[Adam Barth]

Comment on attachment 62371 [details]
patch v1, adding a getActiveFrame method to get current active JS execute frame

WebCore/bindings/js/ScriptController.cpp:306
 +      return exec ? toDynamicFrame(exec) : 0;
This is dangerous.  The lexical frame is most likely to be the active frame.  The dynamic frame isn't used for very much beyond user gesture.

WebCore/loader/FrameLoader.cpp:1133
 +      Frame* enteredFrame = frame->script()->getActiveFrame();
We don't use the term "enteredFrame" outside the V8 bindings.

WebCore/loader/FrameLoader.cpp:1137
 +      return enteredFrame->script()->processingUserGesture(mainThreadNormalWorld()); // FIXME: Use pageIsProcessingUserGesture.
This is all way too complicated.  Every call site will need to do this dance.  Can't we push this junk into the bindings / ScriptController?

[Yury Semikhatsky]

(In reply to comment #0)
> Created an attachment (id=62300) [details]
> 
> My solution is adding a function in ScriptController to get the active/entered frame, so the function FrameLoader::isProcessingUserGesture can correctly test the user gesture status of the page.
> 
> Any comments?
> 

I think that we shouldn't make conclusion based on the last entered frame. There may be several entered frames on the stack and it may happen that one of them processing user gesture is not the topmost one. 

I suggest isProcessingUserGesture should be a property of the owning Page. In that case we could check it from any frame and if any of the frames is processing user gesture the test would succeed for any frame in the page.


(In reply to comment #0)
> Created an attachment (id=62300) [details]
> On Chromium, the flag WebCore::Setting::m_javaScriptCanOpenWindowsAutomatically is always true, the request of opening window is always passed to browser process to do the popup check. When enabling chromium's popup blocker, only user-initiated open request can be allowed.
> 
What I don't understand is why m_javaScriptCanOpenWindowsAutomatically is always true in Chromium if we want to allow only user initiated popups.

[Johnny(Jianning) Ding]

(In reply to comment #8)
> I think that we shouldn't make conclusion based on the last entered frame. There may be several entered frames on the stack and it may happen that one of them processing user gesture is not the topmost one. 

Yes, there may be several entered frames(V8 uses a list to save all entered frames), and it is possible that the frame which is processing user gesture is not the topmost one. But the entered frame we get from JS engine is the last entered frame, the event which can be treated as user gesture seems to only happen in that last entered frame. I may be wrong since I am not the engine expert, please correct me if I am wrong.
In my attached test case, the click event fired on the iframe, no matter which window finally opened the new popup, the last entered frame we get in chromium is always the frame of iframe element.

> I suggest isProcessingUserGesture should be a property of the owning Page. In that case we could check it from any frame and if any of the frames is processing user gesture the test would succeed for any frame in the page.

I agree with you that the isProcessingUserGesture should only return the user-gesture status of the owning Page, not status of another page, that is why I proposed to add another function in ScriptController.

(In reply to comment #8)
> What I don't understand is why m_javaScriptCanOpenWindowsAutomatically is always true in Chromium if we want to allow only user initiated popups.

That is because Chromium want to get all popup requests and judge them in browser process. In Chromium, we implement the popup blocked in the browser process and judge the popup requests according to their user-gesture status. If one popup request is not user initiated and user does not set exception rule for it, chromium will deny the request, otherwise the request will be granted (Please refer to TabContents::AddNewContents and TabContents::AddPopup, We will change the behavior of popup blocker a little bit according to bug http://crbug.com/38458)

[Johnny(Jianning) Ding]

Thanks for review, Adam!

(In reply to comment #7)
> (From update of attachment 62371 [details])
> WebCore/bindings/js/ScriptController.cpp:306
>  +      return exec ? toDynamicFrame(exec) : 0;
> This is dangerous.  The lexical frame is most likely to be the active frame.  The dynamic frame isn't used for very much beyond user gesture.

I don't exactly know the difference between lexicalFrame and dynamicFrame. My only knowledge about those frame is from the comments in CallFrame.h(line 49-54).
According to the comments, the dynamicFrame is the frame in which execution began. lexicalFrame is  the frame in which current executing code was defined.
Differs from dynamicGlobalObject() during function calls across web browser frames. 
So if we define the window.open in the top frame and call the top frame's window.open in sub frame(iframe) by mouse click. The dynamicFrame seems like a right frame to test its user gesture status.
Also WebKit JS binding mostly use dynamicFrame to test the user gesture, please refer to JSDOMWindowCustom.cpp(line 520, 750, 764, 813), JSLocationCustom.cpp(line 194, 328).
also because JSDOMWindow::open test dynamicFrame's user gesture, my attached case can not affect WebKit.

> 
> WebCore/loader/FrameLoader.cpp:1133
>  +      Frame* enteredFrame = frame->script()->getActiveFrame();
> We don't use the term "enteredFrame" outside the V8 bindings.
Done.

> WebCore/loader/FrameLoader.cpp:1137
>  +      return enteredFrame->script()->processingUserGesture(mainThreadNormalWorld()); // FIXME: Use pageIsProcessingUserGesture.
> This is all way too complicated.  Every call site will need to do this dance.  Can't we push this junk into the bindings / ScriptController?

I see, yes, we should hide those details in bindings/ScriptController. Only question is whether it needs to be implemented inside ScriptController::processingUserGesture. According to Yury's change and his comments, seems like the processingUserGesture should be a property of the owning Page. How about adding a new method like activeFrameIsProcessingUserGesture and use it to test active frame's user gesture. Otherwise can we use ScriptController::anyPageIsProcessingUserGesture() to test whether any frame is prcessing user gsture.

[Adam Barth]

> I don't exactly know the difference between lexicalFrame and dynamicFrame.

You're right that we use the dynamic frame for gestures, but we use the lexical frame for almost everything else.  Calling the dynamic frame the "active frame" is likely to confuse people into screwing up other uses.

>How about adding a new method like activeFrameIsProcessingUserGesture and use it to test active frame's user gesture. Otherwise can we use ScriptController::anyPageIsProcessingUserGesture() to test whether any frame is prcessing user gsture.

I don't understand why we need these two APIs.  Can you explain why we need to make these distinctions?  It seems like clients just want to know if we're processing a user gesture.

[Johnny(Jianning) Ding]

(In reply to comment #11)
> 
> You're right that we use the dynamic frame for gestures, but we use the lexical frame for almost everything else.  Calling the dynamic frame the "active frame" is likely to confuse people into screwing up other uses.

I see, sorry for the confused wording, I will change it.
 
> I don't understand why we need these two APIs.  Can you explain why we need to make these distinctions?  It seems like clients just want to know if we're processing a user gesture.

If processingUserGesture is a instance method of ScriptController, it should reflect user gesture status of the owning Page and the caller should take responsibility to find the right Frame/ScriptController before calling the method. But unfortunately not every call follows this convention.
I saw that you and Yury had discussed this issues in https://bugs.webkit.org/show_bug.cgi?id=41350 before.

Like you suggest before, if we define processingUserGesture as a static member function of ScriptController, it makes sense to say it reflects whether WebKit is processing user gesture no matter which frame.
After searching all references of ScriptController::processingUserGesture in the WebCore, so far looks like every call of ScriptController::processingUserGesture just only wants to know whether WebKit is processing user gesture, they seems not to care which frame is processing.

Only thing we need to worry about is whether checking user gesture status of dynamic frame in ScriptController::processingUserGesture can cause regressions of bug 41350 & 41511. It's because the patches of those two bugs introduced the current issue. 

@Yury, According to the comments in crbug.com/44070, looks like the root cause of the bug 41350 was not directly related to getting the dynamic/entered frame in ScriptController::processingUserGesture. Are you OK with defining the function as static member function of ScriptController and checking the user gesture status of the dynamic/entered frame?

[Johnny(Jianning) Ding]

Created attachment 63047 [details]
patch V2, change ScriptController::processingUserGesture to static 

I changed the code according to Adam's comments,  put the whole logic inside ScriptController::processingUserGesture and make it as static.
I have run the webkit layout tests(OSX 10.5) and chromium UI test, didn't see any regressions. So this patch should not affect  bug 41350 & bug 41511.

[WebKit Review Bot]

Attachment 63047 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/bindings/v8/ScriptController.cpp:165:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/bindings/v8/ScriptController.cpp:166:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/bindings/v8/ScriptController.cpp:167:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/bindings/v8/ScriptController.cpp:168:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/bindings/v8/ScriptController.cpp:169:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/bindings/js/ScriptController.cpp:251:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/bindings/js/ScriptController.cpp:252:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/bindings/js/ScriptController.cpp:253:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/bindings/js/ScriptController.cpp:254:  Tab found; better to use spaces  [whitespace/tab] [1]
Total errors found: 9 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Johnny(Jianning) Ding]

Created attachment 63048 [details]
fix style warnings

[Darin Adler]

Comment on attachment 63048 [details]
fix style warnings

If all the callers are figuring out the current word from the ExecState, and ScriptController::processingUserGesture is going to get the exec state from JSMainThreadExecState::currentState then I don't think the world should be passed in to ScriptController. Why can't ScriptController call currentWorld?

[Darin Adler]

Current "world", not "word".

[Yury Semikhatsky]

(In reply to comment #12)
> @Yury, According to the comments in crbug.com/44070, looks like the root cause of the bug 41350 was not directly related to getting the dynamic/entered frame in ScriptController::processingUserGesture. Are you OK with defining the function as static member function of ScriptController and checking the user gesture status of the dynamic/entered frame?

The problem was in the assumption that any entered context has an internal field pointing to V8DOMWindow which is wrong in case of devtools utility context. In the scenario which I was trying to fix we had an inspector JS code calling inspector bindings from the utility context and then native inspector code invoking some method that required isProcessingUserGesture check. The check would end up with an assertion failure in V8Proxy::retrieveWindow(because there is not pointer to V8DOMWindow from the utility context). Making ScriptController::processingUserGesture not depend on the entered context looked like a good solution and I continue to think that it shouldn't depend on the entered context. ScriptController has access to the Frame and to its owning Page which should be enough to figure out whether there is an active user gesture. 

There is already a static method UserGestureIndicator::processingUserGesture() and having another one with the same name  also static looks confusing.

[Yury Semikhatsky]

(In reply to comment #12)
> @Yury, According to the comments in crbug.com/44070, looks like the root cause of the bug 41350 was not directly related to getting the dynamic/entered frame in ScriptController::processingUserGesture. Are you OK with defining the function as static member function of ScriptController and checking the user gesture status of the dynamic/entered frame?

Now that http://trac.webkit.org/changeset/63965 changed the way DevTools dispatches messages on the back end so that it doesn't enter the utility context anymore, this change shouldn't break DevTools functionality that I was fixing in my patch.

[Johnny(Jianning) Ding]

(In reply to comment #18)

> The problem was in the assumption that any entered context has an internal field pointing to V8DOMWindow which is wrong in case of devtools utility context. In the scenario which I was trying to fix we had an inspector JS code calling inspector bindings from the utility context and then native inspector code invoking some method that required isProcessingUserGesture check. The check would end up with an assertion failure in V8Proxy::retrieveWindow(because there is not pointer to V8DOMWindow from the utility context). Making ScriptController::processingUserGesture not depend on the entered context looked like a good solution and I continue to think that it shouldn't depend on the entered context. ScriptController has access to the Frame and to its owning Page which should be enough to figure out whether there is an active user gesture. 

@Yury, Thanks for your detailed explanation, Yury!
Yes, ScriptController has access to its owning Frame to check whether there is an active user gesture. But the user gesture can be initiated from another frame, I agree with Adam that in most situation clients just want to know if we're processing a user gesture, so a unified interface to get user gesture info looks more convenient.

@Adam, any comments for my patch V2 except Darin's comment? I'd like to get more comments before sending the new patch.

[Johnny(Jianning) Ding]

(In reply to comment #19)

> Now that http://trac.webkit.org/changeset/63965 changed the way DevTools dispatches messages on the back end so that it doesn't enter the utility context anymore, this change shouldn't break DevTools functionality that I was fixing in my patch.

Thanks, Yury!

[Johnny(Jianning) Ding]

(In reply to comment #16)
> (From update of attachment 63048 [details])
> If all the callers are figuring out the current word from the ExecState, and ScriptController::processingUserGesture is going to get the exec state from JSMainThreadExecState::currentState then I don't think the world should be passed in to ScriptController. Why can't ScriptController call currentWorld?

Thanks Darin, you are right, will change it in the new patch

[Adam Barth]

Comment on attachment 63048 [details]
fix style warnings

WebCore/bindings/js/ScriptController.cpp:249
 +      ExecState* exec = JSMainThreadExecState::currentState();
Why don't we pass in the exec state?  (All the call sites I see so far seem in have an exec state.)

WebCore/bindings/v8/ScriptController.cpp:208
 +  bool ScriptController::anyPageIsProcessingUserGesture() const
Seems like we should just remove this method since it's the same as processingUserGesture().

[Johnny(Jianning) Ding]

(In reply to comment #23)
> (From update of attachment 63048 [details])
> WebCore/bindings/js/ScriptController.cpp:249
>  +      ExecState* exec = JSMainThreadExecState::currentState();
> Why don't we pass in the exec state?  (All the call sites I see so far seem in have an exec state.)
We passed the DOMWrapperWorld to ScriptController::processingUserGesture in both V8 binding and JSC binding. So we can call the ScriptController::processingUserGesture in other WebCore code regardless of which binding WebKit used.
If we change to pass the exec state to ScriptController::processingUserGesture, since there is no exec state in V8 binding, we can not call ScriptController::processingUserGesture in WebCore (for example, the call in FrameLoader) except we create a wrapper object for exec state in V8.
As Darin suggested, we can drop the DOMWrapperWorld parameter (V8 never use it) in both two bindings.


> WebCore/bindings/v8/ScriptController.cpp:208
>  +  bool ScriptController::anyPageIsProcessingUserGesture() const
> Seems like we should just remove this method since it's the same as processingUserGesture().
In V8 binding, yes, anyPageIsProcessingUserGesture is the same as processingUserGesture, 
But looking the FIXME comment, looks like the original author is not confident for this implementation, and also anyPageIsProcessingUserGesture has its own implementation in JSC binding. So I am not sure we should remove this function in V8 binding in the patch of this bug.

[Adam Barth]

Ok.  I didn't see any call sites from webcore proper in this patch (I might have missed them).  Webcores view of exec state is called scriptstate, which exists for both jsc and v8.  That's proabaly not appropriate to use in this case, but I mention it in case its usefulness to you in the future.

[Johnny(Jianning) Ding]

(In reply to comment #25)
> Ok.  I didn't see any call sites from webcore proper in this patch (I might have missed them).  Webcores view of exec state is called scriptstate, which exists for both jsc and v8.  That's proabaly not appropriate to use in this case, but I mention it in case its usefulness to you in the future.

I see, Thanks, Adam!

[Johnny(Jianning) Ding]

Created attachment 63451 [details]
patch V3

[Adam Barth]

Comment on attachment 63451 [details]
patch V3

WebCore/WebCore.order:7344
 +  __ZN7WebCore16ScriptController21processingUserGestureEv�
There appears to be a strange character here.

WebCore/bindings/js/JSDOMBinding.cpp:682
 +  bool processingUserGesture()
Maybe remove all callers of this method?  They just need to call the ScriptController version.

[Johnny(Jianning) Ding]

Created attachment 63525 [details]
patch v3 to remove strange character in previous patch

(In reply to comment #28)
> (From update of attachment 63451 [details])
> WebCore/WebCore.order:7344
>  +  __ZN7WebCore16ScriptController21processingUserGestureEv�
> There appears to be a strange character here.
Done

> WebCore/bindings/js/JSDOMBinding.cpp:682
>  +  bool processingUserGesture()
> Maybe remove all callers of this method?  They just need to call the ScriptController version.
The processingUserGesture in JSDOMBinding.cpp is used in derived source code and all derived source files do not include ScriptController.h, so processingUserGesture looks like a wrapper function to call the implementation function without introducing more head files. We may need to keep it.


The new patch is same as the previous one yury approved except I removed the strange character in previous patch. So I didn't add the review mark.
If we are all satisfied with this one, would you please help on landing? Thanks!

[Adam Barth]

Comment on attachment 63525 [details]
patch v3 to remove strange character in previous patch

Ok.  We'll probably want to clean up the global function in a later patch.  The commit-queue is a bit backed up at the moment, but it should clear out eventually.

http://queues.webkit.org/queue-status/commit-queue

[Johnny(Jianning) Ding]

(In reply to comment #30)
> (From update of attachment 63525 [details])
> Ok.  We'll probably want to clean up the global function in a later patch.  The commit-queue is a bit backed up at the moment, but it should clear out eventually.
> 
> http://queues.webkit.org/queue-status/commit-queue

OK, Thanks, Adam!

[WebKit Commit Bot]

Comment on attachment 63525 [details]
patch v3 to remove strange character in previous patch

Rejecting patch 63525 from commit-queue.

Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Adam Barth', u'--force']" exit_code: 2
Last 500 characters of output:
file LayoutTests/fast/events/popup-blocked-from-iframe-script-expected.txt
patching file LayoutTests/fast/events/popup-blocked-from-iframe-script.html
patch unexpectedly ends in middle of line
patch: **** malformed patch at line 32:  

fatal: pathspec 'LayoutTests/fast/events/popup-blocked-from-iframe-script.html' did not match any files
Failed to git add LayoutTests/fast/events/popup-blocked-from-iframe-script.html. at /Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply line 427.

Full output: http://queues.webkit.org/results/3636412

[Johnny(Jianning) Ding]

(In reply to comment #32)
> (From update of attachment 63525 [details])
> Rejecting patch 63525 from commit-queue.
> 
> Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Adam Barth', u'--force']" exit_code: 2
> Last 500 characters of output:
> file LayoutTests/fast/events/popup-blocked-from-iframe-script-expected.txt
> patching file LayoutTests/fast/events/popup-blocked-from-iframe-script.html
> patch unexpectedly ends in middle of line
> patch: **** malformed patch at line 32:  
> 
> fatal: pathspec 'LayoutTests/fast/events/popup-blocked-from-iframe-script.html' did not match any files
> Failed to git add LayoutTests/fast/events/popup-blocked-from-iframe-script.html. at /Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply line 427.
> 
> Full output: http://queues.webkit.org/results/3636412

Hi Adam, Eric
I am not sure the reason of the commit failure, is that because I missed a new line at the end of 'LayoutTests/fast/events/popup-blocked-from-iframe-script.html'?
The patch was generated by svn-create-patch, I didn't see any error/warning info at that time,.

[Adam Barth]

Hum... Not sure.  The commit-queue just uses "svn-apply --force" in a git checkout, so you can test it yourself if you like.  Otherwise, I can try landing your patch myself.

[Johnny(Jianning) Ding]

(In reply to comment #34)
> Hum... Not sure.  The commit-queue just uses "svn-apply --force" in a git checkout, so you can test it yourself if you like.  Otherwise, I can try landing your patch myself.

Thanks for your super quick response, Adam!
Now I can not access my mac computer. I will check it later, then I will ask for your help.
Thanks again!

[Eric Seidel (no email)]

Comment on attachment 63451 [details]
patch V3

Cleared Yury Semikhatsky's review+ from obsolete attachment 63451 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Johnny(Jianning) Ding]

Hi Adam, 
I have tested the patch in my local svn checkout (I used the patch cmd), no patch error was generated, no new test was broken. Please help me to land my patch again.
Thanks!

[Adam Barth]

Ok.  I just kicked off a manual land.

[Adam Barth]

Looks like your patch conflicts with something that just landed.  Probably http://trac.webkit.org/changeset/64991

$ patch -p0 < patch 
patching file WebCore/ChangeLog
Hunk #1 FAILED at 1.
1 out of 1 hunk FAILED -- saving rejects to file WebCore/ChangeLog.rej
patching file WebCore/WebCore.order
patching file WebCore/bindings/js/JSDOMBinding.cpp
patching file WebCore/bindings/js/JSDOMBinding.h
patching file WebCore/bindings/js/JSDOMWindowCustom.cpp
patching file WebCore/bindings/js/JSDocumentCustom.cpp
patching file WebCore/bindings/js/JSLocationCustom.cpp
patching file WebCore/bindings/js/ScriptController.cpp
patching file WebCore/bindings/js/ScriptController.h
patching file WebCore/bindings/scripts/CodeGeneratorJS.pm
Hunk #1 succeeded at 1968 (offset 17 lines).
patching file WebCore/bindings/scripts/test/JS/JSTestObj.cpp
Hunk #1 succeeded at 1064 (offset 39 lines).
Hunk #2 succeeded at 1088 (offset 39 lines).
patching file WebCore/bindings/v8/ScriptController.cpp
patching file WebCore/bindings/v8/ScriptController.h
patching file WebCore/bindings/v8/V8Utilities.cpp
Hunk #1 FAILED at 92.
1 out of 1 hunk FAILED -- saving rejects to file WebCore/bindings/v8/V8Utilities.cpp.rej
patching file WebCore/bindings/v8/custom/V8DOMWindowCustom.cpp
Hunk #1 FAILED at 454.
1 out of 1 hunk FAILED -- saving rejects to file WebCore/bindings/v8/custom/V8DOMWindowCustom.cpp.rej
patching file WebCore/loader/FrameLoader.cpp
Hunk #1 succeeded at 1129 (offset -1 lines).
patching file LayoutTests/ChangeLog
Hunk #1 FAILED at 1.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/ChangeLog.rej
patching file LayoutTests/fast/events/popup-blocked-from-iframe-script-expected.txt
patching file LayoutTests/fast/events/popup-blocked-from-iframe-script.html
patch unexpectedly ends in middle of line
patch: **** malformed patch at line 440:

[Yury Semikhatsky]

(In reply to comment #39)
> Looks like your patch conflicts with something that just landed.  Probably http://trac.webkit.org/changeset/64991
> 
> $ patch -p0 < patch 
> patching file WebCore/ChangeLog
> Hunk #1 FAILED at 1.
> ...
> patching file LayoutTests/fast/events/popup-blocked-from-iframe-script.html
> patch unexpectedly ends in middle of line
> patch: **** malformed patch at line 440:

The patch expects extra EOL at the end of patch file. I see the same problem locally.

[Yury Semikhatsky]

(In reply to comment #40)
> (In reply to comment #39)
> > Looks like your patch conflicts with something that just landed.  Probably http://trac.webkit.org/changeset/64991
> > 
> > $ patch -p0 < patch 
> > patching file WebCore/ChangeLog
> > Hunk #1 FAILED at 1.
> > ...
> > patching file LayoutTests/fast/events/popup-blocked-from-iframe-script.html
> > patch unexpectedly ends in middle of line
> > patch: **** malformed patch at line 440:
> 
> The patch expects extra EOL at the end of patch file. I see the same problem locally.
(besides the conflicts with other changes, of cause)

[Johnny(Jianning) Ding]

Created attachment 64006 [details]
patch v3 to resolve conflicts

(In reply to comment #39)
> Looks like your patch conflicts with something that just landed.  Probably http://trac.webkit.org/changeset/64991
Thanks Adam! 
Yes, it conflicted with http://trac.webkit.org/changeset/64991.
I have resolved all conflicts and tested again.  Now uploading the new patch. Please review it and help landing if it's OK.

(In reply to comment #40)
> The patch expects extra EOL at the end of patch file. I see the same problem locally.
Thanks, Yury! I have rewritten the html file and used the hex editor to make sure every line has LF(0x0A). Hopefully it can resolve this issue.

[Johnny(Jianning) Ding]

Sorry for setting the wrong review flag:-(

[Adam Barth]

Comment on attachment 64006 [details]
patch v3 to resolve conflicts

BTW, you don't need to change WebCore.order.  I'm not sure what that file does, but it's managed by some automated process.

[Eric Seidel (no email)]

WebKit.order is updated by Apple before shipping.  It's an optimization thing.  I you want to know more about order files, see -order_file in man ld(1):

http://developer.apple.com/mac/library/documentation/Darwin/Reference/ManPages/man1/ld.1.html

[Adam Barth]

Committed r65082: <http://trac.webkit.org/changeset/65082>

[Johnny(Jianning) Ding]

(In reply to comment #45)
> WebKit.order is updated by Apple before shipping.  It's an optimization thing.  I you want to know more about order files, see -order_file in man ld(1):
> 
> http://developer.apple.com/mac/library/documentation/Darwin/Reference/ManPages/man1/ld.1.html

Thanks, Eric!
I changed the order file was because I was trapped by a link error (not in this patch) which cost me a lot of time to figure out, it was caused by order file unchanged.

[Eric Seidel (no email)]

You may sometimes have to *remove* symbols from the order file (because I believe it's a link error if symbols are listed there but not present in the binary).  But you never have to add them.

[Johnny(Jianning) Ding]

(In reply to comment #48)
> You may sometimes have to *remove* symbols from the order file (because I believe it's a link error if symbols are listed there but not present in the binary).  But you never have to add them.

I see. Thanks!