Closed
Bug 1245499
Opened 8 years ago
Closed 8 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)
Core
Layout
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)
Comment 1•8 years ago
|
||
(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)
Comment 2•8 years ago
|
||
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
Comment 3•8 years ago
|
||
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)
Updated•8 years ago
|
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
Comment 4•8 years ago
|
||
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)
Comment 5•8 years ago
|
||
Thanks Kamil! Is it possible that we are facing a regression caused by Bug 686281? Just guessing here! https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2b73b0a4d52bbd72ef8d8720256b0bba59c7de01&tochange=a152a1cbdcf0b2221e03f1d65ee23e6a01e50bac
Flags: needinfo?(cku)
Comment 6•8 years ago
|
||
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
Comment 7•8 years ago
|
||
Adding some more SVG-folks to get that problem assigned and resolved!
Reporter | ||
Comment 8•8 years ago
|
||
dholbert, any chance you can take a look?
Flags: needinfo?(dholbert)
Reporter | ||
Updated•8 years ago
|
Flags: needinfo?(dholbert)
Comment 9•8 years ago
|
||
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.
Comment 10•8 years ago
|
||
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.
Comment 11•8 years ago
|
||
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.)
Comment 12•8 years ago
|
||
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
Updated•8 years ago
|
Blocks: 686281
Keywords: regression
Blocks: mask-image
Assignee | ||
Comment 13•8 years ago
|
||
I am working it from today.
Assignee | ||
Comment 14•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/35987/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/35987/
Assignee | ||
Comment 15•8 years ago
|
||
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/
Assignee | ||
Comment 16•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/36093/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/36093/
Assignee | ||
Comment 17•8 years ago
|
||
Try result https://treeherder.mozilla.org/#/jobs?repo=try&revision=acf91a7463c4
Assignee | ||
Comment 18•8 years ago
|
||
(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
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.
Assignee | ||
Comment 20•8 years ago
|
||
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)
Updated•8 years ago
|
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)
Comment 22•8 years ago
|
||
Jet, this one seems to have gotten stuck, can you help unstuck this one?
Flags: needinfo?(bugs)
Assignee | ||
Comment 23•8 years ago
|
||
Not stuck, I am heading back to mask relative bugs, including to this one, after bug 1261578 landed.
Comment hidden (spam) |
Comment hidden (spam) |
Comment hidden (spam) |
Attachment #8748231 -
Attachment is obsolete: true
Comment hidden (spam) |
Attachment #8722342 -
Flags: review?(dbaron) → review?(dholbert)
Comment 28•8 years ago
|
||
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)
Assignee | ||
Comment 29•8 years ago
|
||
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)
Comment 30•8 years ago
|
||
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...?
Comment 31•8 years ago
|
||
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)
Assignee | ||
Comment 32•8 years ago
|
||
In old-configure.in, remove dnl in this line dnl MOZ_ENABLE_MASK_AS_SHORTHAND=1 ./mach clobber ./mach build
Flags: needinfo?(cku)
Assignee | ||
Comment 33•8 years ago
|
||
if (NS_SUCCEEDED(rv) && isEqualExceptRef) { return nullptr; << set a breakpoint here } You will see #mask-ring-cutout immediately
Comment 34•8 years ago
|
||
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.)
Comment 35•8 years ago
|
||
(by "This function" I mean "the function you're tweaking in the latest patch", of course.)
Assignee | ||
Comment 36•8 years ago
|
||
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
Assignee | ||
Comment 37•8 years ago
|
||
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 38•8 years ago
|
||
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)
Comment 39•8 years ago
|
||
(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
Comment 40•8 years ago
|
||
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)...
Comment 41•8 years ago
|
||
Tracking, recent regression,
status-firefox46:
--- → ?
status-firefox48:
--- → affected
status-firefox49:
--- → affected
tracking-firefox47:
--- → +
tracking-firefox48:
--- → +
tracking-firefox49:
--- → +
Comment 42•8 years ago
|
||
Removing flags from FF47 & FF48 as the masking features have been disabled from those releases.
tracking-firefox47:
+ → ---
tracking-firefox48:
+ → ---
Comment 43•8 years ago
|
||
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.)
Assignee | ||
Comment 44•8 years ago
|
||
(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.
Assignee | ||
Comment 45•8 years ago
|
||
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)
Assignee | ||
Comment 46•8 years ago
|
||
Actually, it's all about CSS_PROPERTY_START_IMAGE_LOADS flag in mask-image
Assignee | ||
Comment 47•8 years ago
|
||
(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>
Comment 48•8 years ago
|
||
(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).
Updated•8 years ago
|
Flags: needinfo?(cku)
Comment 49•8 years ago
|
||
(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.)
Assignee | ||
Comment 50•8 years ago
|
||
(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 51•8 years ago
|
||
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+
Assignee | ||
Comment 52•8 years ago
|
||
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
Comment 54•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6cf4da71b921
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in
before you can comment on or make changes to this bug.
Description
•