Bug 24337 - Assert when doing sync XHR in worker for cacheable response.
Summary: Assert when doing sync XHR in worker for cacheable response.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: XML (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: David Levin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-03-03 15:39 PST by David Levin
Modified: 2009-12-23 14:38 PST (History)
2 users (show)

See Also:


Attachments
Proposed fix. (6.82 KB, patch)
2009-03-03 17:15 PST, David Levin
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Levin 2009-03-03 15:39:43 PST
For example:
    req = new XMLHttpRequest;
    req.open("GET", "missing-file", false);
    req.send();
Comment 1 David Levin 2009-03-03 16:29:22 PST
I also found a way to get this same assert when doing a cross site async xhr request in a document.

I'll attach both test cases and the fix.
Comment 2 David Levin 2009-03-03 17:15:47 PST
Created attachment 28249 [details]
Proposed fix.
Comment 3 Alexey Proskuryakov 2009-03-04 00:57:40 PST
Comment on attachment 28249 [details]
Proposed fix.

+            if (window.layoutTestController)
+                layoutTestController.notifyDone();
+            log("PASS");

The PASS string is printed after notifyDone(), so I'm very surprised it shows up in expected results.

+    if (m_sendResourceLoadCallbacks)
+        return frameLoader()->client()->willCacheResponse(documentLoader(), identifier(), response);
+    return 0;

Our usual style is to do early return (if (!m_sendResourceLoadCallbacks) return 0;). Some say that's bad for performance though, so we may need to reconsider it eventually.

I don't think this has much to do with the resource being missing - more likely, it's just that the response is cacheable, unlike what a CGI from methods.html sends. It would be nice to make this clearer in ChangeLog and test comments.

r=me
Comment 4 David Levin 2009-03-04 14:31:47 PST
Committed as r41434.

Comment 5 Robert Hogan 2009-12-23 14:38:54 PST
> +        * http/tests/xmlhttprequest/access-control-basic-denied-preflight-cache-expected.txt: Added.
> +        * http/tests/xmlhttprequest/access-control-basic-denied-preflight-cache.html: Added.

Hi David,

As you can see from https://bugs.webkit.org/show_bug.cgi?id=32521 I'm a bit confused about what the above test is meant to prove. I should have looked it up in the logs a long time ago!

If I'm reading the bug correctly, it is possible to perform an async xhr where m_sendResourceLoadCallbacks is false and in such cases this test will ensure it doesn't trigger an assert on the Mac port.

On Qt and Gtk at least, m_sendResourceLoadCallbacks is true when this test is run and it wouldn't touch the patched code in any event. So is this test Mac specific, or is it testing something in more general WebCore?

Thanks for any light you can shed on this.