Closed Bug 732178 Opened 8 years ago Closed 8 years ago

CORS redirect handling broken when image cache validation is happening

Categories

(Core :: ImageLib, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla13
Tracking Status
firefox-esr10 --- unaffected

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(2 files)

We clobber the channel notification callbacks with the cache validator, which means the CORS code will never see the redirect.

I don't know how to write a test that exercises this, sadly....
Whiteboard: [need review]
Comment on attachment 602096 [details] [diff] [review]
Set up the image cache validator before the CORS listener.

Review of attachment 602096 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/src/imgLoader.cpp
@@ -1294,5 @@
>  
>        listener = corsproxy;
>      }
>  
> -    newChannel->SetNotificationCallbacks(hvc);

You could also just change this to SetNotificationCallbacks(listener). Might need a QueryInterface, though, and thus might not be worthwhile.

I don't know whether that'll cause trouble with an nsCORSListenerProxy that fails to initialize in its constructor, though; maybe Jonas can weigh in on that.
Attachment #602096 - Flags: review?(joe) → review+
Comment on attachment 602096 [details] [diff] [review]
Set up the image cache validator before the CORS listener.

Review of attachment 602096 [details] [diff] [review]:
-----------------------------------------------------------------

They way this patch does it is the right way. You have to set up the notifications callback before constructing the CORS proxy since the proxy ctor grabs the existing notification listener.
Attachment #602096 - Flags: review?(jonas) → review+
Oh, and I'd still love to know how to test this.

I pushed to try, and at first glance it looks like this patch is leaking on reftests (or at least all debug reftests are orange with leaks of 3 nsCORSListenerProxy and 3 imgCacheEntry and 3 imgRequest).

Oddly enough, none of the leaked nsStandardURLs are obvious CORS-enabled images:

  file:///Users/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/ogg-video/poster-2.html
  file:///Users/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/ogg-video/not-a-valid-file
  file:///Users/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/webm-video/poster-2.html
  file:///Users/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/webm-video/not-a-valid-file
  file:///Users/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/svg/image/image-svg-inline-01.html

There is also an nsSimpleURI leaked....
Actually, nevermind.  That may be fallout from bug 696301.  Though the fact that we get leaks there is .... disturbing.
Yes, indeed.  The issue was bug 696301 plus a prexisting leak in imagelib.  Filed bug 732319 on that.
http://hg.mozilla.org/integration/mozilla-inbound/rev/593ae5487293
Flags: in-testsuite?
Whiteboard: [need review]
Target Milestone: --- → mozilla13
https://hg.mozilla.org/mozilla-central/rev/593ae5487293
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
From our read of this bug, we don't believe ESR 10 is affected. Please email if that's not the case.
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.