Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

CEF3: Move LoadRequest to the browser process, use data: URL for LoadString #579

Closed
magreenblatt opened this issue Apr 24, 2012 · 47 comments
Closed

CEF3: Move LoadRequest to the browser process, use data: URL for LoadString #579

magreenblatt opened this issue Apr 24, 2012 · 47 comments
Labels
enhancement Enhancement request Framework Related to framework code or APIs

Comments

@magreenblatt
Copy link
Collaborator

Original report by Anonymous.

Original issue 579 created by nesf.fs on 2012-04-24T22:20:20.000Z:

What steps will reproduce the problem?
Replace calls to CefFrame::LoadURL with CefFrame::LoadRequest. This can be accomplished by creating a CefRequest object, calling SetURL with a given URL, and passing said object to CefFrame::LoadRequest. A convenient place to do this is in the cefclient_win.cpp file used for the cefclient test app.

What is the expected output? What do you see instead?
If the steps above are followed, we should navigate to a different page when a different URL is entered on the address bar. Instead, nothing happens.

What version of the product are you using? On what operating system?
CEF3, revision 588. Windows 7.

Please provide any additional information below.
This was first discovered through resolving http://magpcss.org/ceforum/viewtopic.php?f=6&t=784

I believe the problem is somewhere in CefBrowserHostImpl::LoadRequest, but I'm not sure where the problem is, exactly.
@magreenblatt
Copy link
Collaborator Author

Comment 1. originally posted by magreenblatt on 2012-05-22T15:43:37.000Z:

You need to load at least one URL before you can use LoadRequest. For example, you can pass "about:blank" as the |url| parameter to CefBrowserHost::CreateBrowser. Once the browser has been created you can then call LoadRequest(). See RequestSendRecvTestHandler in tests/unittests/request_unittest.cc for an example.

@magreenblatt
Copy link
Collaborator Author

  • set state to "open"

@magreenblatt
Copy link
Collaborator Author

Comment 2. originally posted by magreenblatt on 2012-09-24T15:40:13.000Z:

issue #727 has been merged into this issue.

@magreenblatt
Copy link
Collaborator Author

Comment 3. originally posted by magreenblatt on 2012-09-24T15:41:25.000Z:

There is currently no unit test for the LoadString case but it's likely the same problem.

@magreenblatt
Copy link
Collaborator Author

Comment 4. originally posted by magreenblatt on 2012-11-26T18:06:23.000Z:

issue #797 has been merged into this issue.

@magreenblatt
Copy link
Collaborator Author

Comment 5. originally posted by magreenblatt on 2013-10-16T14:35:51.000Z:

issue #797 has been merged into this issue.

@magreenblatt
Copy link
Collaborator Author

Original comment by Anonymous.

Comment 6. originally posted by alervdvcw on 2013-11-20T04:36:52.000Z:

I experience this problem too.

@magreenblatt
Copy link
Collaborator Author

Original comment by Anonymous.

Comment 7. originally posted by nvineeth on 2014-01-28T12:58:16.000Z:

In CEF3 branch 1650 the following problem is reproducible :

  1. Navigate to a page say google.com
  2. Navigate to a non-existing address, ex: http://insert-random-junk-chars-here.com
  3. cefclient enters infinite loop where OnLoadError is called due to previous frame->LoadString() in client_handler.cpp ClientHandler::OnLoadError

@magreenblatt
Copy link
Collaborator Author

Comment 8. originally posted by magreenblatt on 2014-05-20T15:46:29.000Z:

issue #1277 has been merged into this issue.

@magreenblatt
Copy link
Collaborator Author

Comment 9. originally posted by magreenblatt on 2014-05-20T15:48:41.000Z:

LoadRequest can be implemented from the browser process using NavigationController::LoadURLWithParams. The LoadURLParams structure supports extra headers, post data and target frame.

@magreenblatt
Copy link
Collaborator Author

Comment 10. originally posted by magreenblatt on 2014-05-20T15:50:40.000Z:

issue #641 has been merged into this issue.

@magreenblatt
Copy link
Collaborator Author

Comment 11. originally posted by magreenblatt on 2014-05-20T15:51:15.000Z:

@ commentcomment 10.: Also set should_clear_history_list via LoadRequest from the browser process.

@magreenblatt
Copy link
Collaborator Author

Original comment by gberman (Bitbucket: Gennady.Berman).

Comment 12. originally posted by gberman on 2014-06-27T16:03:14.000Z:

I have a patch for LoadString and LoadRequest to go through NavigationController::LoadURLWithParams. I ran into a some problems supporting all the existing options for LoadRequest:

  1. No support for first_party_for_cookies
  2. No support for load_flags
  3. LoadURLWithParams expects a vector for the body, so no support for multi-part body

To support LoadString() I converted the string content into a data: url

@magreenblatt
Copy link
Collaborator Author

Original changes by gberman (Bitbucket: Gennady.Berman).

  • set attachment to "cef_patch.txt"

@magreenblatt
Copy link
Collaborator Author

Comment 13. originally posted by magreenblatt on 2014-06-27T16:39:33.000Z:

@ comment 12.: Thanks for the patch.

I ran into a some problems supporting all the existing options for LoadRequest:

  1. No support for first_party_for_cookies
  2. No support for load_flags
  3. LoadURLWithParams expects a vector for the body, so no support for multi-part body

These should be fine. |load_flags| is primarily intended for use with CefURLRequest in any case.

To support LoadString() I converted the string content into a data: url

This is a nice solution.

Some comments about your patch:

  1. Please add a unit test for LoadString.

  2. https://code.google.com/p/chromiumembedded/issues/attachmentText?id=579&aid=5790012000&name=cef\_patch.txt&token=ABZ6GAdoKLA17EoEMbvKRfoYk1YW885PaQ%3A1403885760761comment 48.8

  • IPC_MESSAGE_HANDLER(CefMsg_LoadRequest, LoadRequestParams)

A. Rename CefMsg_LoadRequest to CefHostMsg_LoadRequest because it is now sent from the render process to the browser process.
B. Remove the CefMsg_LoadRequest handler from CefBrowserImpl because it's no longer used.
C. Remove members from CefHostMsg_LoadRequest that are no longer supported.

  1. https://code.google.com/p/chromiumembedded/issues/attachmentText?id=579&aid=5790012000&name=cef\_patch.txt&token=ABZ6GAdoKLA17EoEMbvKRfoYk1YW885PaQ%3A1403885760761comment 16.6
  • content::NavigationController::LoadURLParams params(
  •  GetGURL(url));

Indent the 2nd+ lines 4 spaces instead of 2. Same with other locations.

  1. https://code.google.com/p/chromiumembedded/issues/attachmentText?id=579&aid=5790012000&name=cef\_patch.txt&token=ABZ6GAdoKLA17EoEMbvKRfoYk1YW885PaQ%3A1403885760761comment 29.5

+};
} // namespace

Add a space between end of class and end of namespace.

@magreenblatt
Copy link
Collaborator Author

Original comment by gberman (Bitbucket: Gennady.Berman).

Comment 14. originally posted by gberman on 2014-06-27T16:56:05.000Z:

@ 13: Thanks for the feedback. I'll work on your suggestions. Quick comment on:

2B. Remove the CefMsg_LoadRequest handler from CefBrowserImpl because it's no longer used.

This is still used for navigating from the render process in frame_impl.cc

@magreenblatt
Copy link
Collaborator Author

Comment 15. originally posted by magreenblatt on 2014-06-27T16:58:59.000Z:

@ comment 14.: But the message is no longer sent from the browser process to the render process, right? So you don't need to handle the message in CefBrowserImpl::OnMessageReceived.

@magreenblatt
Copy link
Collaborator Author

Original comment by gberman (Bitbucket: Gennady.Berman).

Comment 16. originally posted by gberman on 2014-06-27T17:01:42.000Z:

@ comment 15.: Yup, I understand now. Thanks

@magreenblatt
Copy link
Collaborator Author

Original comment by gberman (Bitbucket: Gennady.Berman).

Comment 17. originally posted by gberman on 2014-06-30T16:36:19.000Z:

@ comment 12.: I'm having some trouble writing up a unit test for LoadString.

  1. None of the CefRequestHandler methods (OnBeforeResourceLoad, GetResourceHandler, etc...) seem to be triggered by LoadString. So I can't compare the CefRequest to the expected data url (similar to the SendRecv test in request_test.cc)

  2. I've tried using CefFrame::GetSource during OnLoadEnd, but the CefStringVisitor::Visit method doesn't get triggered either. I'm not sure if there is a timing issue causing this since the visitor is meant to be called asynchronously. I can see the content getting loading the browser window so it is there but perhaps not during OnLoadEnd.

Do you have any suggestions on now to implement a unit test for LoadString?

@magreenblatt
Copy link
Collaborator Author

Comment 18. originally posted by magreenblatt on 2014-06-30T16:48:11.000Z:

@ comment 17.: For 2, are you waiting for the VIsit method to be called asynchronously?

@magreenblatt
Copy link
Collaborator Author

Original comment by gberman (Bitbucket: Gennady.Berman).

Comment 19. originally posted by gberman on 2014-06-30T16:52:32.000Z:

@ comment 17.: I'm not sure how to do that. As an aside, is there a way to run an individual test without running through the entire test suite?

Thanks,

@magreenblatt
Copy link
Collaborator Author

Comment 20. originally posted by magreenblatt on 2014-06-30T16:58:53.000Z:

@ comment 19.:

I'm not sure how to do that.

Create a TestHandler that also implements CefStringVisitor. Call CefFrame::GetSource from inside OnLoadEnd using |this| as the visitor. Call DestroyTest from the Visit implementation after verifying that the contents are correct.

As an aside, is there a way to run an individual test without running through the entire test suite?

Pass --gtest_filter=TestType.TestName on the command-line.

@magreenblatt
Copy link
Collaborator Author

Original comment by gberman (Bitbucket: Gennady.Berman).

Comment 21. originally posted by gberman on 2014-06-30T18:02:59.000Z:

@ comment 20.: Excellent... I got this working but my impl is a bit hacky. See attached.

I couldn't create a TestHandler that implements CefStringVisitor directly, because CefFrame::GetSource takes in a CefRefPtr. So I can't pass in |this| into GetSource.

Is there a CEF equivalent to boost::enable_shared_from_this I could use? Or do you have any other idea on how to clean this up?

@magreenblatt
Copy link
Collaborator Author

Original changes by gberman (Bitbucket: Gennady.Berman).

  • set attachment to "load_string_test.txt"

@magreenblatt
Copy link
Collaborator Author

Original comment by gberman (Bitbucket: Gennady.Berman).

Comment 22. originally posted by gberman on 2014-06-30T18:35:05.000Z:

Here's the actual patch for the load string test

@magreenblatt
Copy link
Collaborator Author

Original changes by gberman (Bitbucket: Gennady.Berman).

  • set attachment to "load_string_test.txt"

@magreenblatt
Copy link
Collaborator Author

Comment 23. originally posted by magreenblatt on 2014-06-30T19:09:13.000Z:

@ comment 21.: You should be able to assign |this| to a CefRefPtr, and then pass in the CefRefPtr. For example: CefRefPtr visitor(this);

@magreenblatt
Copy link
Collaborator Author

Original comment by gberman (Bitbucket: Gennady.Berman).

Comment 24. originally posted by gberman on 2014-06-30T19:35:57.000Z:

@ comment 23.: That works. Here's the updated patch for LoadString. Thanks again for the feedback.

@magreenblatt
Copy link
Collaborator Author

Original changes by gberman (Bitbucket: Gennady.Berman).

  • set attachment to "cef_patch.txt"

@magreenblatt
Copy link
Collaborator Author

Comment 25. originally posted by magreenblatt on 2014-06-30T20:25:54.000Z:

@ comment 24.: Looks good, thanks.

@magreenblatt
Copy link
Collaborator Author

Original comment by gberman (Bitbucket: Gennady.Berman).

Comment 26. originally posted by gberman on 2014-07-01T20:46:50.000Z:

Any chance this can be patched into the 1916 branch?

@magreenblatt
Copy link
Collaborator Author

Original changes by gberman (Bitbucket: Gennady.Berman).

  • set attachment to "cef_patch_1916.txt"

@magreenblatt
Copy link
Collaborator Author

Comment 27. originally posted by magreenblatt on 2014-07-02T21:46:50.000Z:

@ comment 26.: Yes, it will be merged to 1916 as well.

@magreenblatt
Copy link
Collaborator Author

Comment 28. originally posted by magreenblatt on 2014-07-02T22:25:56.000Z:

issue #763 has been merged into this issue.

@magreenblatt
Copy link
Collaborator Author

Comment 29. originally posted by magreenblatt on 2014-07-02T22:26:16.000Z:

@magreenblatt
Copy link
Collaborator Author

Comment 30. originally posted by magreenblatt on 2014-07-02T22:58:53.000Z:

issue #1185 has been merged into this issue.

@magreenblatt
Copy link
Collaborator Author

Comment 31. originally posted by magreenblatt on 2014-07-10T18:24:18.000Z:

@ comment 24.: Added in trunk revision 1765 and 1916 branch revision 1766 with minor changes.

@magreenblatt
Copy link
Collaborator Author

  • set state to "resolved"

@magreenblatt
Copy link
Collaborator Author

Comment 32. originally posted by magreenblatt on 2014-07-16T21:05:56.000Z:

@ comment 31.: The changes will be reverted due to the following problems (see also issue comment 13.39):

  1. WebContentsImpl::NavigateToPendingEntry ignores the specified frame_tree_node_id unless the "--site-per-process" flag is set. This mode is still experimental and not fully implemented so we can't use it yet.

  2. The frame_id value (which comes from RenderFrame routing ID) is not the same as frame_tree_node_id. NavigationHelper will need to convert from frame_id to frame_tree_node_id, perhaps using FrameTree::FindByRoutingID. Otherwise, |frame_tree_.FindByID| in WebContentsImpl::NavigateToPendingEntry returns NULL.

  3. Need to add unit tests for the sub-frame loading use case.

@magreenblatt
Copy link
Collaborator Author

  • set state to "open"

@magreenblatt
Copy link
Collaborator Author

Comment 33. originally posted by magreenblatt on 2014-07-16T21:06:48.000Z:

issue #1339 has been merged into this issue.

@magreenblatt
Copy link
Collaborator Author

Comment 34. originally posted by magreenblatt on 2014-07-16T21:29:17.000Z:

@ comment 32.: Reverted in trunk revision 1780 and 1916 branch revision 1781.

Also:

  1. When re-applying the changes to trunk remove the FilePathStringToWebString function from browser_impl.cc since it is no longer used.

@magreenblatt
Copy link
Collaborator Author

Comment 35. originally posted by magreenblatt on 2015-02-11T18:17:17.000Z:

Trunk revision 2028 and 2272 branch revision 2029 update cefclient to use a data: URI instead of LoadString for loading error messages. This also adds base64 and URI encoding/decoding functions to cef_url.h.

@magreenblatt
Copy link
Collaborator Author

  • edited description
  • changed kind from "bug" to "enhancement"
  • set component to "Framework"

@magreenblatt
Copy link
Collaborator Author

Duplicate of #2290.

@magreenblatt
Copy link
Collaborator Author

  • changed state from "new" to "duplicate"

@magreenblatt
Copy link
Collaborator Author

This will be resolved as part of issue #2290.

filipnavara pushed a commit to emclient/cef that referenced this issue Dec 26, 2023
@magreenblatt
Move LoadRequest execution to the browser process and use data: URLs …
bd4d9d3 
…for LoadString (issue chromiumembedded#579).

git-svn-id: https://chromiumembedded.googlecode.com/svn/branches/1916@1766 5089003a-bbd8-11dd-ad1f-f1f9622dbc98
filipnavara pushed a commit to emclient/cef that referenced this issue Dec 26, 2023
@magreenblatt
Revert revision 1766 changes due to broken sub-frame loading.
0bc4471 
- Move LoadRequest execution to the browser process and use data: URLs for LoadString (issue chromiumembedded#579).

git-svn-id: https://chromiumembedded.googlecode.com/svn/branches/1916@1781 5089003a-bbd8-11dd-ad1f-f1f9622dbc98
filipnavara pushed a commit to emclient/cef that referenced this issue Dec 26, 2023
@magreenblatt
- Pass SSL certificate information to CefRequestHandler::OnCertificat…
32d4ca3 
…eError via a new CefSSLInfo interface (issue chromiumembedded#1530).

- cefclient: Improve error message text and use a data: URI instead of LoadString for loading error messages (issue chromiumembedded#579).
- Add functions in cef_url.h for base64 and URI encoding/decoding (issue chromiumembedded#579).

git-svn-id: https://chromiumembedded.googlecode.com/svn/branches/2272@2029 5089003a-bbd8-11dd-ad1f-f1f9622dbc98
@magreenblatt magreenblatt mentioned this issue Feb 21, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement Enhancement request Framework Related to framework code or APIs
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@magreenblatt