Closed Bug 1245499 Opened 4 years ago Closed 3 years ago

Security Error: Content at chrome://browser/skin/identity-icon.svg#normal attempted to load chrome://browser/skin/identity-icon.svg#mask-ring-cutout, but may not load external data when being used as an image.

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox46 --- unaffected
firefox47 --- unaffected
firefox48 --- unaffected
firefox49 + unaffected

People

(Reporter: Gijs, Assigned: u459114)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

STR:

1. open Firefox
2. open browser console

ER:
no security errors

AR:
"Security Error: Content at chrome://browser/skin/identity-icon.svg#normal attempted to load chrome://browser/skin/identity-icon.svg#mask-ring-cutout, but may not load external data when being used as an image."


Christoph, do you know what's up here?
Flags: needinfo?(mozilla)
(In reply to :Gijs Kruitbosch from comment #0)
> STR:
> 
> 1. open Firefox
> 2. open browser console
> 
> ER:
> no security errors
> 
> AR:
> "Security Error: Content at chrome://browser/skin/identity-icon.svg#normal
> attempted to load chrome://browser/skin/identity-icon.svg#mask-ring-cutout,
> but may not load external data when being used as an image."
> 
> 
> Christoph, do you know what's up here?

I have seen that the first time myself yesterday - I will investigate. Very likely a regression caused by converting channel callsites to use asyncOpen2() rather than asyncOpen.
Flags: needinfo?(mozilla)
Here is a first stacktrace, it seems it's not related to asyncOpen2(), because we haven't touched images yet, so it must be something else:

#0  0x00007ff844d5ef3d in nanosleep () at ../sysdeps/unix/syscall-template.S:81
#1  0x00007ff844d5edd4 in __sleep (seconds=0) at ../sysdeps/unix/sysv/linux/sleep.c:137
#2  0x00007ff8377c03d6 in ah_crap_handler (signum=11) at /home/ckerschb/moz/mc/toolkit/xre/nsSigHandlers.cpp:103
#3  0x00007ff83779dae0 in nsProfileLock::FatalSignalHandler (signo=11, info=0x7ffd1f6d73b0, context=0x7ffd1f6d7280)
    at /home/ckerschb/moz/mc/toolkit/profile/nsProfileLock.cpp:191
#4  0x00007ff8389ad0df in AsmJSFaultHandler (signum=11, info=0x7ffd1f6d73b0, context=0x7ffd1f6d7280) at /home/ckerschb/moz/mc/js/src/asmjs/WasmSignalHandlers.cpp:1157
#5  <signal handler called>
#6  nsDataDocumentContentPolicy::ShouldLoad (this=0x7ff8249d5860, aContentType=3, aContentLocation=0x7ff80ffa6100, aRequestingLocation=0x7ff811f83c00, 
    aRequestingContext=0x7ff810cc5000, aMimeGuess=..., aExtra=0x0, aRequestPrincipal=0x7ff81bcfee80, aDecision=0x7ffd1f6d8206)
    at /home/ckerschb/moz/mc/dom/base/nsDataDocumentContentPolicy.cpp:107
#7  0x00007ff8342639d7 in nsContentPolicy::CheckPolicy (this=0x7ff826d69470, 
    policyMethod=&virtual nsIContentPolicy::ShouldLoad(unsigned int, nsIURI*, nsIURI*, nsISupports*, nsACString_internal const&, nsISupports*, nsIPrincipal*, short*), 
    simplePolicyMethod=&virtual table offset 24, contentType=37, contentLocation=0x7ff80ffa6100, requestingLocation=0x7ff811f83c00, requestingContext=0x7ff810cc5000, 
    mimeType=..., extra=0x0, requestPrincipal=0x7ff81bcfee80, decision=0x7ffd1f6d8206) at /home/ckerschb/moz/mc/dom/base/nsContentPolicy.cpp:140
#8  0x00007ff83424b1b8 in nsContentPolicy::ShouldLoad (this=0x7ff826d69470, contentType=37, contentLocation=0x7ff80ffa6100, requestingLocation=0x7ff811f83c00, 
    requestingContext=0x7ff810cc5000, mimeType=..., extra=0x0, requestPrincipal=0x7ff81bcfee80, decision=0x7ffd1f6d8206)
    at /home/ckerschb/moz/mc/dom/base/nsContentPolicy.cpp:241
#9  0x00007ff833f74085 in NS_CheckContentLoadPolicy (contentType=37, contentLocation=0x7ff80ffa6100, originPrincipal=0x7ff81bcfee80, context=0x7ff810cc5000, mimeType=..., 
    extra=0x0, decision=0x7ffd1f6d8206, policyService=0x7ff826d69470, aSecMan=0x7ff82db4add0) at /home/ckerschb/moz/mc/dom/base/nsContentPolicyUtils.h:237
#10 0x00007ff83402d225 in nsContentUtils::CanLoadImage (aURI=0x7ff80ffa6100, aContext=0x7ff810cc5000, aLoadingDocument=0x7ff810cc5000, aLoadingPrincipal=0x7ff81bcfee80, 
    aImageBlockingStatus=0x0, aContentType=37) at /home/ckerschb/moz/mc/dom/base/nsContentUtils.cpp:3023
#11 0x00007ff8367a1f55 in mozilla::css::ImageLoader::LoadImage (this=0x7ff811dfbbc0, aURI=0x7ff80ffa6100, aOriginPrincipal=0x7ff81bcfee80, aReferrer=0x7ff811f83c00, 
    aImage=0x7ff80fbc8240) at /home/ckerschb/moz/mc/layout/style/ImageLoader.cpp:260
#12 0x00007ff836861018 in mozilla::css::ImageValue::ImageValue (this=0x7ff80fbc8240, aURI=0x7ff80ffa6100, aString=0x7ff82449c6a0, aReferrer=0x7ff811f83c00, 
    aOriginPrincipal=0x7ff81bcfee80, aDocument=0x7ff810cc5000) at /home/ckerschb/moz/mc/layout/style/nsCSSValue.cpp:2557
#13 0x00007ff83685cf0b in nsCSSValue::StartImageLoad (this=0x7ff80fed2970, aDocument=0x7ff810cc5000) at /home/ckerschb/moz/mc/layout/style/nsCSSValue.cpp:705
#14 0x00007ff8368213c3 in TryToStartImageLoadOnValue (aValue=..., aDocument=0x7ff810cc5000, aContext=0x7ff811084de0, aProperty=eCSSProperty_mask_image, aForTokenStream=false)
    at /home/ckerschb/moz/mc/layout/style/nsCSSDataBlock.cpp:108
#15 0x00007ff8368212ab in TryToStartImageLoad (aValue=..., aDocument=0x7ff810cc5000, aContext=0x7ff811084de0, aProperty=eCSSProperty_mask_image, aForTokenStream=false)
    at /home/ckerschb/moz/mc/layout/style/nsCSSDataBlock.cpp:153
#16 0x00007ff83682120b in TryToStartImageLoad (aValue=..., aDocument=0x7ff810cc5000, aContext=0x7ff811084de0, aProperty=eCSSProperty_mask_image, aForTokenStream=false)
    at /home/ckerschb/moz/mc/layout/style/nsCSSDataBlock.cpp:143
#17 0x00007ff8367e436a in MapSinglePropertyInto (aSrcProp=eCSSProperty_mask_image, aSrcValue=0x7ff80fbc52f8, aTargetProp=eCSSProperty_mask_image, aTargetValue=0x7ffd1f6d8720, 
    aRuleData=0x7ffd1f6d88b0) at /home/ckerschb/moz/mc/layout/style/nsCSSDataBlock.cpp:213
#18 0x00007ff8367e3dbd in nsCSSCompressedDataBlock::MapRuleInfoInto (this=0x7ff80fbc52f0, aRuleData=0x7ffd1f6d88b0)
    at /home/ckerschb/moz/mc/layout/style/nsCSSDataBlock.cpp:351
#19 0x00007ff83678e00c in mozilla::css::Declaration::MapRuleInfoInto (this=0x7ff80fbb90c0, aRuleData=0x7ffd1f6d88b0) at /home/ckerschb/moz/mc/layout/style/Declaration.cpp:98
#20 0x00007ff8368aa620 in nsRuleNode::WalkRuleTree (this=0x7ff811084e90, aSID=eStyleStruct_SVGReset, aContext=0x7ff811084de0)
    at /home/ckerschb/moz/mc/layout/style/nsRuleNode.cpp:2306
#21 0x00007ff835511d08 in nsRuleNode::GetStyleSVGReset<true> (this=0x7ff811084e90, aContext=0x7ff811084de0)
    at /home/ckerschb/moz/mc-obj-dbg/dist/include/nsStyleStructList.h:142
#22 0x00007ff835511b8f in nsStyleContext::DoGetStyleSVGReset<true> (this=0x7ff811084de0) at /home/ckerschb/moz/mc-obj-dbg/dist/include/nsStyleStructList.h:142
#23 0x00007ff8354f6c05 in nsStyleContext::StyleSVGReset (this=0x7ff811084de0) at /home/ckerschb/moz/mc-obj-dbg/dist/include/nsStyleStructList.h:142
#24 0x00007ff8358e2bd2 in nsIFrame::StyleSVGReset (this=0x7ff80fb22020) at /home/ckerschb/moz/mc-obj-dbg/dist/include/nsStyleStructList.h:142
#25 0x00007ff836b40964 in nsFrame::DidSetStyleContext (this=0x7ff80fb22020, aOldStyleContext=0x0) at /home/ckerschb/moz/mc/layout/generic/nsFrame.cpp:817
#26 0x00007ff836b3fb75 in nsFrame::Init (this=0x7ff80fb22020, aContent=0x7ff815a78830, aParent=0x7ff811084678, aPrevInFlow=0x0)
    at /home/ckerschb/moz/mc/layout/generic/nsFrame.cpp:592
#27 0x00007ff836c0e742 in nsSplittableFrame::Init (this=0x7ff80fb22020, aContent=0x7ff815a78830, aParent=0x7ff811084678, aPrevInFlow=0x0)
---Type <return> to continue, or q <return> to quit---
    at /home/ckerschb/moz/mc/layout/generic/nsSplittableFrame.cpp:22
#28 0x00007ff836b27a52 in nsContainerFrame::Init (this=0x7ff80fb22020, aContent=0x7ff815a78830, aParent=0x7ff811084678, aPrevInFlow=0x0)
    at /home/ckerschb/moz/mc/layout/generic/nsContainerFrame.cpp:53
#29 0x00007ff836cd53c3 in nsSVGDisplayContainerFrame::Init (this=0x7ff80fb22020, aContent=0x7ff815a78830, aParent=0x7ff811084678, aPrevInFlow=0x0)
    at /home/ckerschb/moz/mc/layout/svg/nsSVGContainerFrame.cpp:137
#30 0x00007ff836cef9c8 in nsSVGGFrame::Init (this=0x7ff80fb22020, aContent=0x7ff815a78830, aParent=0x7ff811084678, aPrevInFlow=0x0)
    at /home/ckerschb/moz/mc/layout/svg/nsSVGGFrame.cpp:40
#31 0x00007ff836d13c50 in nsSVGUseFrame::Init (this=0x7ff80fb22020, aContent=0x7ff815a78830, aParent=0x7ff811084678, aPrevInFlow=0x0)
    at /home/ckerschb/moz/mc/layout/svg/nsSVGUseFrame.cpp:112
#32 0x00007ff8369bc6a0 in nsCSSFrameConstructor::InitAndRestoreFrame (this=0x7ff80feae520, aState=..., aContent=0x7ff815a78830, aParentFrame=0x7ff811084678, 
    aNewFrame=0x7ff80fb22020, aAllowCounters=true) at /home/ckerschb/moz/mc/layout/base/nsCSSFrameConstructor.cpp:4763
#33 0x00007ff8369c4a8e in nsCSSFrameConstructor::ConstructFrameFromItemInternal (this=0x7ff80feae520, aItem=..., aState=..., aParentFrame=0x7ff811084678, aFrameItems=...)
    at /home/ckerschb/moz/mc/layout/base/nsCSSFrameConstructor.cpp:3774
#34 0x00007ff8369c92d1 in nsCSSFrameConstructor::ConstructFramesFromItem (this=0x7ff80feae520, aState=..., aIter=..., aParentFrame=0x7ff811084678, aFrameItems=...)
    at /home/ckerschb/moz/mc/layout/base/nsCSSFrameConstructor.cpp:5943
#35 0x00007ff836a1bdfe in nsCSSFrameConstructor::ConstructFramesFromItemList (this=0x7ff80feae520, aState=..., aItems=..., aParentFrame=0x7ff811084678, aFrameItems=...)
    at /home/ckerschb/moz/mc/layout/base/nsCSSFrameConstructor.cpp:10264
#36 0x00007ff8369cf5d2 in nsCSSFrameConstructor::ContentAppended (this=0x7ff80feae520, aContainer=0x7ff812a1fcc0, aFirstNewContent=0x7ff80fddae00, 
    aAllowLazyConstruction=false) at /home/ckerschb/moz/mc/layout/base/nsCSSFrameConstructor.cpp:7263
#37 0x00007ff8369ccd2c in nsCSSFrameConstructor::CreateNeededFrames (this=0x7ff80feae520, aContent=0x7ff812a1fcc0)
    at /home/ckerschb/moz/mc/layout/base/nsCSSFrameConstructor.cpp:6905
#38 0x00007ff8369cf9da in nsCSSFrameConstructor::CreateNeededFrames (this=0x7ff80feae520) at /home/ckerschb/moz/mc/layout/base/nsCSSFrameConstructor.cpp:6927
#39 0x00007ff836979907 in mozilla::RestyleManager::ProcessPendingRestyles (this=0x7ff811e34640) at /home/ckerschb/moz/mc/layout/base/RestyleManager.cpp:1728
#40 0x00007ff836a8ccd1 in PresShell::FlushPendingNotifications (this=0x7ff80febe400, aFlush=...) at /home/ckerschb/moz/mc/layout/base/nsPresShell.cpp:3982
#41 0x00007ff836a8b8e3 in PresShell::FlushPendingNotifications (this=0x7ff80febe400, aType=Flush_Layout) at /home/ckerschb/moz/mc/layout/base/nsPresShell.cpp:3870
#42 0x00007ff833fa19c5 in mozilla::image::SVGDocumentWrapper::FlushLayout (this=0x7ff80fed1d80) at /home/ckerschb/moz/mc/image/SVGDocumentWrapper.cpp:391
#43 0x00007ff833facbec in mozilla::image::VectorImage::OnSVGDocumentLoaded (this=0x7ff811017aa0) at /home/ckerschb/moz/mc/image/VectorImage.cpp:1171
#44 0x00007ff833fb8fdb in mozilla::image::SVGLoadEventListener::HandleEvent (this=0x7ff811dc6e20, aEvent=0x7ff824491f20) at /home/ckerschb/moz/mc/image/VectorImage.cpp:224
#45 0x00007ff8356dc3c3 in mozilla::EventListenerManager::HandleEventSubType (this=0x7ff811018160, aListener=0x7ff811f78858, aDOMEvent=0x7ff824491f20, 
    aCurrentTarget=0x7ff810cc5000) at /home/ckerschb/moz/mc/dom/events/EventListenerManager.cpp:1064
#46 0x00007ff8356dcc97 in mozilla::EventListenerManager::HandleEventInternal (this=0x7ff811018160, aPresContext=0x7ff810cc5800, aEvent=0x7ff80fbc81d0, 
    aDOMEvent=0x7ffd1f6da0e8, aCurrentTarget=0x7ff810cc5000, aEventStatus=0x7ffd1f6da0f0) at /home/ckerschb/moz/mc/dom/events/EventListenerManager.cpp:1224
#47 0x00007ff8356d6e86 in mozilla::EventListenerManager::HandleEvent (this=0x7ff811018160, aPresContext=0x7ff810cc5800, aEvent=0x7ff80fbc81d0, aDOMEvent=0x7ffd1f6da0e8, 
    aCurrentTarget=0x7ff810cc5000, aEventStatus=0x7ffd1f6da0f0) at /home/ckerschb/moz/mc-obj-dbg/dist/include/mozilla/EventListenerManager.h:349
#48 0x00007ff8356ca9d6 in mozilla::EventTargetChainItem::HandleEvent (this=0x7ff82514c008, aVisitor=..., aCd=...) at /home/ckerschb/moz/mc/dom/events/EventDispatcher.cpp:222
#49 0x00007ff8356c457d in mozilla::EventTargetChainItem::HandleEventTargetChain (aChain=..., aVisitor=..., aCallback=0x0, aCd=...)
    at /home/ckerschb/moz/mc/dom/events/EventDispatcher.cpp:315
#50 0x00007ff8356c5df7 in mozilla::EventDispatcher::Dispatch (aTarget=0x7ff810cc5000, aPresContext=0x7ff810cc5800, aEvent=0x7ff80fbc81d0, aDOMEvent=0x7ff824491f20, 
    aEventStatus=0x7ffd1f6da37c, aCallback=0x0, aTargets=0x0) at /home/ckerschb/moz/mc/dom/events/EventDispatcher.cpp:653
#51 0x00007ff8356b62a7 in mozilla::EventDispatcher::DispatchDOMEvent (aTarget=0x7ff810cc5000, aEvent=0x0, aDOMEvent=0x7ff824491f20, aPresContext=0x7ff810cc5800, 
    aEventStatus=0x7ffd1f6da37c) at /home/ckerschb/moz/mc/dom/events/EventDispatcher.cpp:719
#52 0x00007ff834356a72 in nsINode::DispatchEvent (this=0x7ff810cc5000, aEvent=0x7ff824491f20, aRetVal=0x7ffd1f6da437) at /home/ckerschb/moz/mc/dom/base/nsINode.cpp:1289
#53 0x00007ff8356a6d9e in mozilla::AsyncEventDispatcher::Run (this=0x7ff80ff0e2c0) at /home/ckerschb/moz/mc/dom/events/AsyncEventDispatcher.cpp:51
#54 0x00007ff8326782d3 in nsThread::ProcessNextEvent (this=0x7ff844a83600, aMayWait=false, aResult=0x7ffd1f6da66e) at /home/ckerschb/moz/mc/xpcom/threads/nsThread.cpp:1018
#55 0x00007ff8326f4997 in NS_ProcessNextEvent (aThread=0x7ff844a83600, aMayWait=false) at /home/ckerschb/moz/mc/xpcom/glue/nsThreadUtils.cpp:297
#56 0x00007ff832d872bb in mozilla::ipc::MessagePump::Run (this=0x7ff82f53da00, aDelegate=0x7ff844a6d540) at /home/ckerschb/moz/mc/ipc/glue/MessagePump.cpp:95
#57 0x00007ff832cc5bc5 in MessageLoop::RunInternal (this=0x7ff844a6d540) at /home/ckerschb/moz/mc/ipc/chromium/src/base/message_loop.cc:234
#58 0x00007ff832cc5af5 in MessageLoop::RunHandler (this=0x7ff844a6d540) at /home/ckerschb/moz/mc/ipc/chromium/src/base/message_loop.cc:227
---Type <return> to continue, or q <return> to quit---
#59 0x00007ff832cc5acd in MessageLoop::Run (this=0x7ff844a6d540) at /home/ckerschb/moz/mc/ipc/chromium/src/base/message_loop.cc:201
#60 0x00007ff83655b6e3 in nsBaseAppShell::Run (this=0x7ff826d025c0) at /home/ckerschb/moz/mc/widget/nsBaseAppShell.cpp:156
#61 0x00007ff8376f2527 in nsAppStartup::Run (this=0x7ff826d04060) at /home/ckerschb/moz/mc/toolkit/components/startup/nsAppStartup.cpp:281
#62 0x00007ff8377b5535 in XREMain::XRE_mainRun (this=0x7ffd1f6daf70) at /home/ckerschb/moz/mc/toolkit/xre/nsAppRunner.cpp:4277
#63 0x00007ff8377b5ee6 in XREMain::XRE_main (this=0x7ffd1f6daf70, argc=1, argv=0x7ffd1f6dc558, aAppData=0x7ffd1f6db210)
    at /home/ckerschb/moz/mc/toolkit/xre/nsAppRunner.cpp:4374
#64 0x00007ff8377b6774 in XRE_main (argc=1, argv=0x7ffd1f6dc558, aAppData=0x7ffd1f6db210, aFlags=0) at /home/ckerschb/moz/mc/toolkit/xre/nsAppRunner.cpp:4476
#65 0x0000000000405aa0 in do_main (argc=1, argv=0x7ffd1f6dc558, xreDirectory=0x7ff844a41840) at /home/ckerschb/moz/mc/browser/app/nsBrowserApp.cpp:212
#66 0x00000000004051ec in main (argc=1, argv=0x7ffd1f6dc558) at /home/ckerschb/moz/mc/browser/app/nsBrowserApp.cpp:352
Kamil, any chance you can help me find out the regression window? I think the first time I have seen this problem was roughly 10 days ago.
Flags: needinfo?(kjozwiak)
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
Using "mozregression --good 2016-01-01 --bad 2016-02-05", received the following regression window:

13:04.21 LOG: MainThread Bisector INFO Last good revision: 2b73b0a4d52bbd72ef8d8720256b0bba59c7de01
13:04.21 LOG: MainThread Bisector INFO First bad revision: a152a1cbdcf0b2221e03f1d65ee23e6a01e50bac
13:04.21 LOG: MainThread Bisector INFO Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2b73b0a4d52bbd72ef8d8720256b0bba59c7de01&tochange=a152a1cbdcf0b2221e03f1d65ee23e6a01e50bac

Used the following to confirm:

* mozregression --launch 2016-01-28 <-- doesn't display the error messages under the browser console
* mozregression --launch 2016-01-29 <-- the security error messages are appearing under the browser console
Flags: needinfo?(kjozwiak)
So, it's definitely not caused by any of the asyncOpen2() conversions - reseting Asignee of that bug in that case!
Assignee: mozilla → nobody
Status: ASSIGNED → NEW
Adding some more SVG-folks to get that problem assigned and resolved!
dholbert, any chance you can take a look?
Flags: needinfo?(dholbert)
Assignee: nobody → cku
Flags: needinfo?(cku)
Flags: needinfo?(dholbert)
A few notes:
 - Clearly, from the error message, the resource should not be considered "external" -- it's part of the same document.
 - The code that does the rejection & spams the error message is here:
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDataDocumentContentPolicy.cpp?rev=e22b3043887e&mark=78-91#78
  ...and it only checks flags on the URI that's attempting to be loaded -- it doesn't actually do any URI comparison.


We should probably be doing a nsIURI "equalsExceptRef()" comparison somewhere, though I'm not sure precisely where.

I do wonder how this worked before, too, and what changed to make it not-work. That might provide a hint as to where this comparison should go.
I tested older versions of Firefox and I didn't experience any of those errors when launching Firefox. Kamil actually narrowed down the regression range in Comment 4.
Right, what I meant was "what specific line of code changed to make this not work" (which comment 4 narrows down, but doesn't quite answer).

Anyway, it looks like CJ is assigned here, so hopefully he's already / soon-to-be on the case. (And his masking changes in comment 4's pushlog do look likely-related.)
I tried narrowing down the bisection, hopefully this is more helpful:

2:45.58 INFO: Last good revision: 94a70f108d118a9a22ea6719e1c37867fedf49c7
2:45.58 INFO: First bad revision: 5b135803e145c58116ee8e69d5854e87d3f4ce29
2:45.58 INFO: Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=94a70f108d118a9a22ea6719e1c37867fedf49c7&tochange=5b135803e145c58116ee8e69d5854e87d3f4ce29
Blocks: 686281
Keywords: regression
Blocks: mask-image
I am working it from today.
Comment on attachment 8722342 [details]
MozReview Request: Bug 1245499 - Do not trigger a download request for CSS "mask-image" when it's set to a local-reference URI

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35987/diff/1-2/
(In reply to Daniel Holbert [:dholbert] from comment #9)
> A few notes:
>  - Clearly, from the error message, the resource should not be considered
> "external" -- it's part of the same document.
>  - The code that does the rejection & spams the error message is here:
> http://mxr.mozilla.org/mozilla-central/source/dom/base/
> nsDataDocumentContentPolicy.cpp?rev=e22b3043887e&mark=78-91#78
>   ...and it only checks flags on the URI that's attempting to be loaded --
> it doesn't actually do any URI comparison.
> 
> 
> We should probably be doing a nsIURI "equalsExceptRef()" comparison
> somewhere, though I'm not sure precisely where.
> 
> I do wonder how this worked before, too, and what changed to make it
> not-work. That might provide a hint as to where this comparison should go.
The information here is really useful, thanks, Daniel
Depends on: 1251115
Blocks: mask-ship
Duplicate of this bug: 1251005
Attachment #8722342 - Attachment description: MozReview Request: Bug 1245499 - Do not download SVG document's resoruce while parsing mask attribute. → MozReview Request: Bug 1245499 - Do not download SVG document's resoruce while initiating a style image.
Comment on attachment 8722342 [details]
MozReview Request: Bug 1245499 - Do not trigger a download request for CSS "mask-image" when it's set to a local-reference URI

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35987/diff/2-3/
Attachment #8722563 - Attachment is obsolete: true
Attachment #8722342 - Flags: review?(dbaron)
Component: Networking → Layout
Comment on attachment 8722342 [details]
MozReview Request: Bug 1245499 - Do not trigger a download request for CSS "mask-image" when it's set to a local-reference URI

https://reviewboard.mozilla.org/r/35987/#review33587

So I'm not really an appropriate reviewer for this; I've forgotten how external resource loading works.  I suspect dholbert would be a better reviewer.  Could you redirect?  (Redirecting reviews in MozReview is not implemented yet...)

But, a few comments anyway:

First, please spell resource correctly in the commit message.

Second:

::: layout/style/nsRuleNode.cpp:1230
(Diff revision 3)
> +      {
> +        // You don't need to download svg's resource here, SVGDocument will
> +        // do it automatically.
> +        if (aStyleContext->PresContext()->Document()->AsSVGDocument()) {
> +          break;
> +        }
> +      }

If you're going to have a comment like "SVGDocument will do it automatically", it's better to point to code (e.g., function name) so that people can understand what you're saying.

You also don't need the braces and indentation; there's nothing to scope.
Attachment #8722342 - Flags: review?(dbaron)
Jet, this one seems to have gotten stuck, can you help unstuck this one?
Flags: needinfo?(bugs)
Not stuck, I am heading back to mask relative bugs, including to this one, after bug 1261578 landed.
Flags: needinfo?(bugs)
Attachment #8748231 - Attachment is obsolete: true
Attachment #8722342 - Flags: review?(dbaron) → review?(dholbert)
Comment on attachment 8722342 [details]
MozReview Request: Bug 1245499 - Do not trigger a download request for CSS "mask-image" when it's set to a local-reference URI

https://reviewboard.mozilla.org/r/35987/#review47537

::: layout/style/nsCSSValue.cpp:2580
(Diff revision 4)
> +css::URLValue::IsLocalRef() const
> +{
> +  if (mIsLocalRef.isNothing()) {
> +    if (mString) {
> +      nsString trimmedURL;
> +      mString->ToString(mString->StorageSize() / sizeof(char16_t) - 1,  trimmedURL);

I think tihs StorageSize() usage here is potentially-bogus -- it may include extra junk that is not actually part of the URL. (nsStringBuffer's documentation says it has has null-terminated string data, and its storage size is potentially arbitrarily longer than its actual string contents.)  Maybe it doesn't technically matter, though, since you're just checking the first character.

::: layout/style/nsCSSValue.cpp:2582
(Diff revision 4)
> +  if (mIsLocalRef.isNothing()) {
> +    if (mString) {
> +      nsString trimmedURL;
> +      mString->ToString(mString->StorageSize() / sizeof(char16_t) - 1,  trimmedURL);
> +      trimmedURL.Trim(" ", true, false, false);
> +      mIsLocalRef.emplace(trimmedURL.Length() ? (trimmedURL.First() == '#') : false);

This check (looking for "#" at the start of the string) doesn't seem sufficient.

This check *would* work if, for example, http://[stuff]/foo.svg tries to use "#bar".  But it wouldn't help if it tried to use http://[stuff]/foo.svg#bar . 

And I'm pretty sure those two URLs (#bar &  http://[stuff]/foo.svg#bar ) should be treated the same in this context, from a security/what-gets-loaded perspective.

So/also: I'm not sure this is the correct place to be doing all of this checking. I think we really want to be checking the fully-qualified request-URI against our document's URI, using the "equalsExceptRef()" comparison function or something like it.  And that would need to happen somewhere else -- somewhere where we actually have our document URI available for comparison.

::: layout/style/nsRuleNode.cpp:1235
(Diff revision 4)
>    aResult.SetNull();
>  
>    switch (aValue.GetUnit()) {
>      case eCSSUnit_Image:
> +      // There is no need to download an in-document referenced element.
> +      if (aValue.IsLocalRefURL()){

I expect it would be better (and more robust) to check for this a little bit lower, somewhere inside of the NS_SET_IMAGE_REQUEST_WITH_DOC() invocation (which has the document's URI and which could conceivably check for whether the resource is same-origin or not).
Attachment #8722342 - Flags: review?(dholbert)
Comment on attachment 8722342 [details]
MozReview Request: Bug 1245499 - Do not trigger a download request for CSS "mask-image" when it's set to a local-reference URI

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35987/diff/4-5/
Attachment #8722342 - Attachment description: MozReview Request: Bug 1245499 - Do not download in-document referenced element while initiating a style image. → MozReview Request: Bug 1245499 - do not trigger a download reqeust for local-referenced URI;
Attachment #8722342 - Flags: review?(dholbert)
Does this bug actually still reproduce?
* When I perform this bug's STR in current Nightly (2016-05-05), I'm not seeing this bug's error message in the browser console anymore.
* Similarly, when I directly load the SVG in question (as an image), it also doesn't trigger this bug, e.g. like so:
   data:text/html,<img src="chrome://browser/skin/identity-icon.svg">
* I tried doing both of the above with a Nightly from the day that this bug was filed, and I verified that I _do_ get the error message in both cases with that Nightly. Also, I verified that the text of this SVG file hasn't changed.

So, this might have been fixed incidentally somewhere else...?
Ah, it looks like maybe this feature was just disabled by default in bug 1243734. Does it still reproduce if the flag is enabled at compile time?

What is the correct way to enable that #define at compile time? (sufficiently for all of the relevant #ifdefs to pick it up)  I want to poke around in a debugger to better understand what's going on here.
Flags: needinfo?(cku)
In old-configure.in, remove dnl in this line
dnl MOZ_ENABLE_MASK_AS_SHORTHAND=1

./mach clobber
./mach build
Flags: needinfo?(cku)
    if (NS_SUCCEEDED(rv) && isEqualExceptRef) {
      return nullptr;  << set a breakpoint here
    }

You will see #mask-ring-cutout immediately
Thanks! I'll get back to you by Monday (CA time) I think -- I'm mostly off today, but hope to poke around at this with a debugger over the weekend or on Monday.

(This function -- nsCSSValue::GetImageValue -- seems to be called a lot, and I'm hoping we can avoid having to introduce URL equality-comparisons (i.e. string comparisons) every time it's called.  I'm still betting there's a more targeted place we can perform this check, where it'll only happen once [or a small number of times] -- though I'm not sure where at the moment.)
(by "This function" I mean "the function you're tweaking in the latest patch", of course.)
Here is the call stack
1. nsCSSValue::GetImageValue
   Called by background-image/ mask-image/ border-image/ image etc...
2. SetStyleImage
   Called by background-image/ mask-image/ border-image
   for bg-image, it seems that we can't reference to an in-document SVG element 
   background-image: url(#id-of-svg-element).
   Is it a bug? 
3. BackgroundItemComputer<nsCSSValueList, nsStyleImage>::ComputeValue
   Called by background-image/ mask-image
4. SetImageLayerList<nsStyleImage>
   Called by background-image/ mask-image
5. nsRuleNode::ComputeSVGResetData

I think #2 and #3 are both the right place to do the check, the only difference is border-image is not included in #3
mask used in both HTML and SVG document.

a. mask-image in a HTML document
In this case, we don't need to check whether the url is a local-refereneced-url, since local-refereneced is not valid for a style-image, people won't use it. Do url comparison in this case is simply hurt performance. 
  div {
    mask-image: url(#ref); // not right.
  }
b. a mask in svg document
In this case, local URI reference or absolute URI are both legal.
   *But* we should not trigger any image downloading in SetStyleImage either, no matter it's a local or absolute URI, since it is a job of SVG document itself.
   In [1], we have a mask with local IRI reference.
   <use xlink:href="#shape-circle-base" mask="url(#mask-ring-cutout)" fill="#999" />

That's why I simply skip style image download in [2] while the current document is a SVG document. 

[1] https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/identity-block/identity-icon.svg
[2] https://reviewboard.mozilla.org/r/35987/diff/1/
Comment on attachment 8722342 [details]
MozReview Request: Bug 1245499 - Do not trigger a download request for CSS "mask-image" when it's set to a local-reference URI

https://reviewboard.mozilla.org/r/35987/#review48117

This patch does not fix the bug for me; I still see the error posted to my error console.

(Also, in a debugger, I can see that the clause you're adding doesn't get reached until *after* the image is already rejected, via nsDataDocumentContentPolicy::ShouldLoad -- the backtrace in comment 2 / link in comment 9.  So, that explains why it's not fixing things.)

Looking at the backtrace in comment 2, it looks like nsCSSValue::StartImageLoad() would be a better place to check + skip the load...
Attachment #8722342 - Flags: review?(dholbert)
(In reply to C.J. Ku[:cjku] from comment #36)
>    for bg-image, it seems that we can't reference to an in-document SVG element 
>    background-image: url(#id-of-svg-element).
>    Is it a bug? 

No, that's not a bug.

 * background-image takes an <image>, basically [1].
 * mask-image takes *either* <image> OR <mask-source> [2].
 * Masks with fragment IDs (like the one in question here, #mask-ring-cutout) are a <mask-source>, *not* an <image>.

[1] https://drafts.csswg.org/css-backgrounds-3/#bg-image
[2] https://drafts.fxtf.org/masking/#typedef-mask-reference
The core issue here may be that we're representing a <mask-source> value _as if it were a CSS <image>_.  

Maybe we shouldn't be doing that? We represent it simply as a <url> when MOZ_ENABLE_MASK_AS_SHORTHAND is off. (VARIANT_HUO, "U" = url).

Perhaps in nsCSSValue::StartImageLoad (or somewhere earlier where we have access to the document URI) we should test our tentative-image-URI with equalsExceptRef against the local document -- and if it matches, then do something to ensure we treat it as a <url> instead of an <image> (and take the same codepath that we did before the new MOZ_ENABLE_MASK_AS_SHORTHAND stuff was added)...
Tracking, recent regression,
Removing flags from FF47 & FF48 as the masking features have been disabled from those releases.
Firefox 49 is also unaffected at this point, since (per comment 31 / comment 32) this is disabled by default on trunk right now. (You have to tweak a build configuration file in order to enable the feature that causes the bug.)

(and firefox46 is unaffected as well, since (per comment 5) this only initially regressed when bug 686281 landed, which was for 47.)
(In reply to Daniel Holbert [:dholbert] from comment #40)
> The core issue here may be that we're representing a <mask-source> value _as
> if it were a CSS <image>_.  
> 
> Maybe we shouldn't be doing that? We represent it simply as a <url> when
> MOZ_ENABLE_MASK_AS_SHORTHAND is off. (VARIANT_HUO, "U" = url).
<mask-reference> = none | <image> | <mask-source>

<mask-source> = <url>
<image> = <url> | <image()> | <image-set()> | <element()> | <cross-fade()> | <gradient>

When you have an url, you have no idea it's a <mask-source> or <image> since both of them accept <url>
So we try loading it as an image... then get this error message.
Comment on attachment 8722342 [details]
MozReview Request: Bug 1245499 - Do not trigger a download request for CSS "mask-image" when it's set to a local-reference URI

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35987/diff/5-6/
Attachment #8722342 - Flags: review?(dholbert)
Actually, it's all about CSS_PROPERTY_START_IMAGE_LOADS flag in mask-image
(In reply to Daniel Holbert [:dholbert] from comment #39)
> (In reply to C.J. Ku[:cjku] from comment #36)
> >    for bg-image, it seems that we can't reference to an in-document SVG element 
> >    background-image: url(#id-of-svg-element).
> >    Is it a bug? 
> 
> No, that's not a bug.
> 
>  * background-image takes an <image>, basically [1].
>  * mask-image takes *either* <image> OR <mask-source> [2].
>  * Masks with fragment IDs (like the one in question here,
> #mask-ring-cutout) are a <mask-source>, *not* an <image>.
> 
> [1] https://drafts.csswg.org/css-backgrounds-3/#bg-image
> [2] https://drafts.fxtf.org/masking/#typedef-mask-reference
Just a little bit confuse..
<image> = <url> | ...
Giving url(#id-of-svg-viewbox) to a background-image should be right, since background-image accept <url>
(In reply to C.J. Ku[:cjku] from comment #44)
> When you have an url, you have no idea it's a <mask-source> or <image> since
> both of them accept <url>
> So we try loading it as an image... then get this error message.

So, how/where do we actually find out whether we should treat it as an <image> or a mask reference?

e.g. suppose we have "mask-image: url('someLocalFile#bar'), and "someLocalFile" could be any of the following:
 a) an SVG file with a mask element named #bar.
 b) a PNG file that renders as some shape that the author wants to use as a mask.
 c) an SVG file that renders as some shape that the author wants to use as a mask (with *no* element named #bar)

Do we handle all of those cases correctly? And where do we find out which of those cases we're in? (since they each have different behavior -- or rather, (b) and (c) are similar, but we have to make sure (c) doesn't get grouped in with (a), I'd think).
Flags: needinfo?(cku)
(The latest patch looks like a reasonable way to fix the scenario in this bug, I think -- but I want to make sure I understand how we actually disambiguate URL values here in general, before I sign off on adding this set of special cases.)
(In reply to Daniel Holbert [:dholbert] from comment #48)
> (In reply to C.J. Ku[:cjku] from comment #44)
> > When you have an url, you have no idea it's a <mask-source> or <image> since
> > both of them accept <url>
> > So we try loading it as an image... then get this error message.
> 
> So, how/where do we actually find out whether we should treat it as an
> <image> or a mask reference?
Preconditions:
nsStyleImageLayers::Layers holds 
a. mImage for <image> and
b. mSourceURI for <mask-reference> 

In short, we find it out right before render that mask.
Longer version: 
1. nsSVGIntegrationUtils::PaintFramesWithEffects uses a nsSVGEffects, named effectProperties.
2. effectProperties holds one nsSVGMaskProperty object, named mMask.
3. nsSVGMaskProperty basically keep mSourceURIs of each Layer
4. GenerateMaskSurface[1] is to compose all mask layers onto one single surface.
   In here:
   Calls aEffectProperties.GetMaskFrames() to get a list of nsSVGMaskFrame[2].
   Objects in the list can be
   I.  a nsSVGMaskFrame if we find an reference SVG mask element by #b[3].
   II. Or, nullptr. In this case, we fallback to #a

[1] https://dxr.mozilla.org/mozilla-central/source/layout/svg/nsSVGIntegrationUtils.cpp#430
[2] https://dxr.mozilla.org/mozilla-central/source/layout/svg/nsSVGIntegrationUtils.cpp#439
[3] https://dxr.mozilla.org/mozilla-central/source/layout/svg/nsSVGEffects.cpp#674
> e.g. suppose we have "mask-image: url('someLocalFile#bar'), and
> "someLocalFile" could be any of the following:
>  a) an SVG file with a mask element named #bar.
>  b) a PNG file that renders as some shape that the author wants to use as a
> mask.
>  c) an SVG file that renders as some shape that the author wants to use as a
> mask (with *no* element named #bar)
a) svg mask in an SVG file => 4. I
b) PNG mask => 4. II
c) viewbox in an SVG file=> 4. II
> Do we handle all of those cases correctly? 
Yes, I think so.
Flags: needinfo?(cku)
Comment on attachment 8722342 [details]
MozReview Request: Bug 1245499 - Do not trigger a download request for CSS "mask-image" when it's set to a local-reference URI

https://reviewboard.mozilla.org/r/35987/#review51900

The code here looks good, I think. I've just got several language nits.  r=me with these addressed.

First, a few issues with the commit message:
> Bug 1245499 - do not trigger a download reqeust for local-referenced URI;

(1) "reqeust" is misspelled.
(2) There shouldn't be a semicolon at the end.
(3) Please mention "CSS mask-image" in the commit message, to give some better context about what feature is involved here.
(4) s/referenced/reference/, I think. (The URI *is* a local reference.)

Suggested replacement commit message, which addresses these concerns:
  Bug 1245499 - Don't trigger a download request for CSS "mask-image" when it's set to a local reference URI.

::: layout/style/nsCSSDataBlock.cpp:64
(Diff revision 6)
>  {
>    MOZ_ASSERT(aDocument);
>  
>    if (aValue.GetUnit() == eCSSUnit_URL) {
> +#ifdef MOZ_ENABLE_MASK_AS_SHORTHAND
> +    // Local reference is legal to a mask-image property.

This first sentence is a bit hard to understand. Please reword, maybe as:

    // The 'mask-image' property accepts local reference URIs.

::: layout/style/nsCSSDataBlock.cpp:66
(Diff revision 6)
>  
>    if (aValue.GetUnit() == eCSSUnit_URL) {
> +#ifdef MOZ_ENABLE_MASK_AS_SHORTHAND
> +    // Local reference is legal to a mask-image property.
> +    // For example,
> +    //   mask-image: url(#mask_id); // refer to a SVG mask element, which id is

s/which/whose/

::: layout/style/nsCSSDataBlock.cpp:68
(Diff revision 6)
> +#ifdef MOZ_ENABLE_MASK_AS_SHORTHAND
> +    // Local reference is legal to a mask-image property.
> +    // For example,
> +    //   mask-image: url(#mask_id); // refer to a SVG mask element, which id is
> +    //                              // "mask_id", in the current document.
> +    // While a mask-image referring to an in-document element, no need to trigger

This line is a bit too long, but it also would benefit from a rewording anyway. ("While a mask-image referring" doesn't quite make grammatical sense here.)

Let's reword this comment as follows:

    // For such 'mask-image' values (pointing to an in-document element),
    // there's no need to trigger image download.

::: layout/style/nsRuleNode.cpp:1280
(Diff revision 6)
> +#endif
> +
> +      break;
> +    }
> +    default:
> +      MOZ_ASSERT(false, "Unexpected Unit type.");

MOZ_ASSERT(false,...) is an anti-pattern. Please use the more explict version, "MOZ_ASSERT_UNREACHABLE".  And also, decapitalize "Unit" here.  So, replace with:

    MOZ_ASSERT_UNREACHABLE("Unexpected unit type.");
Attachment #8722342 - Flags: review?(dholbert) → review+
Comment on attachment 8722342 [details]
MozReview Request: Bug 1245499 - Do not trigger a download request for CSS "mask-image" when it's set to a local-reference URI

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35987/diff/6-7/
Attachment #8722342 - Attachment description: MozReview Request: Bug 1245499 - do not trigger a download reqeust for local-referenced URI; → MozReview Request: Bug 1245499 - Do not trigger a download request for CSS "mask-image" when it's set to a local-reference URI
https://hg.mozilla.org/mozilla-central/rev/6cf4da71b921
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.