Bug 146271 - [Content Extensions] Block synchronous XMLHTTPRequests
Summary: [Content Extensions] Block synchronous XMLHTTPRequests
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brady Eidson
Keywords: InRadar
Depends on:
Reported: 2015-06-23 18:27 PDT by Alex Christensen
Modified: 2015-07-02 11:34 PDT (History)
3 users (show)

See Also:

Patch (6.62 KB, patch)
2015-06-23 18:44 PDT, Alex Christensen
beidson: review-
Details | Formatted Diff | Diff
Patch v2 - Taking over from Alex (5.32 KB, patch)
2015-07-01 22:53 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch v3 (5.31 KB, patch)
2015-07-02 08:37 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2015-06-23 18:27:52 PDT
Synchronous loads are currently not blocked.  Let's block them.
Comment 1 Alex Christensen 2015-06-23 18:44:54 PDT
Created attachment 255466 [details]
Comment 2 Alex Christensen 2015-06-23 18:46:41 PDT
It does seem strange to have blocked synchronous requests throw an exception but a asynchronous request just does nothing.  Feedback would be appreciated.
Comment 3 Alexey Proskuryakov 2015-06-23 20:43:36 PDT
Comment on attachment 255466 [details]

View in context: https://bugs.webkit.org/attachment.cgi?id=255466&action=review

> Source/WebCore/ChangeLog:9
> +        http://www.w3.org/TR/2007/WD-XMLHttpRequest-20070618/#exceptions

I don't know if anything changed in this regard, but this is an extremely old draft that is entirely obsolete.

> Source/WebCore/ChangeLog:10
> +        This also adds a test for blocking asynchronous XMLHTTPRequests, which just does not call 

This doesn't seem like good behavior.

> Source/WebCore/ChangeLog:11
> +        onreadystatechange because it never actually sends the request.

Why not handle that as an error case, with onreadystatechange and onerror?
Comment 4 Darin Adler 2015-06-24 15:01:32 PDT
Comment on attachment 255466 [details]

View in context: https://bugs.webkit.org/attachment.cgi?id=255466&action=review

Besides Alexey’s comments, I have some comments about the specifics of the code.

> Source/WebCore/loader/FrameLoader.cpp:3018
> +    String urlForError = newRequest.url().string();

Please don’t compute this unused string.

> Source/WebCore/loader/FrameLoader.cpp:3020
> +    if (m_frame.mainFrame().page() && m_frame.mainFrame().page()->userContentController() && m_documentLoader)
> +        m_frame.mainFrame().page()->userContentController()->processContentExtensionRulesForLoad(*m_frame.mainFrame().page(), newRequest, ResourceType::Raw, *m_documentLoader);

I suggest writing this nested thing:

    if (m_documentLoader) {
        if (auto* page = m_frame.page()) {
            if (auto* controller = page->userContentController())
                controller->processContentExtensionRulesForLoad(*page, newRequest, ResourceType::Raw, *m_documentLoader);
Comment 5 Daniel Bates 2015-06-29 12:20:21 PDT
Comment 6 Brady Eidson 2015-07-01 22:53:03 PDT
Created attachment 255992 [details]
Patch v2 - Taking over from Alex
Comment 7 WebKit Commit Bot 2015-07-01 22:54:37 PDT
Attachment 255992 [details] did not pass style-queue:

ERROR: Source/WebCore/loader/FrameLoader.cpp:95:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 6 files

If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Brady Eidson 2015-07-02 08:37:43 PDT
Created attachment 256011 [details]
Patch v3
Comment 9 WebKit Commit Bot 2015-07-02 11:34:22 PDT
Comment on attachment 256011 [details]
Patch v3

Clearing flags on attachment: 256011

Committed r186228: <http://trac.webkit.org/changeset/186228>
Comment 10 WebKit Commit Bot 2015-07-02 11:34:26 PDT
All reviewed patches have been landed.  Closing bug.