Closed Bug 1258650 Opened 8 years ago Closed 8 years ago

Yahoo finance chart comparison "overlays" not displayed properly because of bad interaction with scaled clip and mask combo

Categories

(Core :: Graphics, defect)

x86_64
All
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla48
Tracking Status
firefox45 - wontfix
firefox46 + verified
firefox47 + verified
firefox48 + verified
firefox-esr45 - wontfix
relnote-firefox --- 46+

People

(Reporter: vmarsi75-bugzilla, Assigned: jrmuizel)

References

()

Details

(Keywords: dev-doc-complete, regression, site-compat, Whiteboard: [gfx-noted])

Attachments

(7 files, 10 obsolete files)

1.42 MB, video/m4v
Details
173.44 KB, image/png
Details
1.08 KB, text/html
Details
491 bytes, text/html
Details
5.73 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
6.20 KB, patch
Details | Diff | Splinter Review
6.19 KB, patch
Details | Diff | Splinter Review
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:45.0) Gecko/20100101 Firefox/45.0
Build ID: 20160315153207

Steps to reproduce:

1. Go to http://finance.yahoo.com/echarts?s=WIPRO.NS+Interactive#{%22range%22:%221d%22,%22allowChartStacking%22:true}
2. Click on the +Comparison "button" on the chart
3. Add any other stock symbol (I added HCLTECH.NS) into the text box



Actual results:

The graph for WIPRO.NS displays alright. The second "comparison" graph is only displayed when the mouse is moved over the chart. As soon as you stop moving the mouse, the comparison graph is not displayed.


Expected results:

The graphs of both stocks should have been displayed even without moving the mouse over the chart.

The problem started to occur only after the latest update of Firefox to 45.0.1
OS: Unspecified → Mac OS X
Hardware: Unspecified → x86_64
Firefox: 45.0.1, Build ID  20160315153207
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:45.0) Gecko/20100101 Firefox/45.0

Hi Ravi,

I have tested this issue on the latest Firefox (45.0.1) release, latest Nightly (48.0a1 - Build ID: 20160322030417) build, but I could not reproduce it. I have followed your provided steps and both graphs are correctly displayed.

Can you please retest this using latest FF release and latest Nightly build (https://nightly.mozilla.org/) and report back the results ? When doing this, please use a new clean Firefox profile, maybe even safe mode, to eliminate custom settings as a possible cause (https://goo.gl/PNe90E). 

Thanks,
Cosmin.
Flags: needinfo?(vmarsi75-bugzilla)
I restarted Firefox in safe-mode and the problem still occurs. I've also tried doing a "refresh firefox" with the same result. Attached is a screen capture showing the problem - notice how the 2nd plot is visible while the mouse is being moved in the chart area but disappears as soon as it is stopped or leaves the chart area. I will try with the nightly shortly and update.
Flags: needinfo?(vmarsi75-bugzilla)
Looks like this problem exists only if the 1d or 5d chart resolutions are chosen. For lower chart resolutions such as 1m, 3m, 6m, etc. the second plot is displayed just fine.

On the nightly: 48.0a1 (2016-03-23) the problem has a slightly different manifestation. The second plot will not appear even if the mouse is moved (in the 1d and 5d resolutions). Lower chart resolutions display fine.
Hi Ravi,

I have tested again and I could reproduce this bug for lower char resolution. I have also performed a regression and this are the results:

Last good revision: 1e5f3d1151d60a1edd6424a35a2e38b5ab17adad
First bad revision: af5c9cf6898b0e2931a809774387cad5953363c3
Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=1e5f3d1151d60a1edd6424a35a2e38b5ab17adad&tochange=af5c9cf6898b0e2931a809774387cad5953363c3

Looks like the following bug has the changes which introduced the regression: bug 1210560

Thanks,
Cosmin.
Status: UNCONFIRMED → NEW
Component: Untriaged → Graphics
Ever confirmed: true
Product: Firefox → Core
Blocks: 1210560
[Tracking Requested - why for this release]: A regression is Firefox 45, affecting SVG charts on Yahoo! Finance.
Hi Milan, Jeff, could you please help find an owner who can investigate this? I thought I'd start with you guys first. Thanks!
Flags: needinfo?(milan)
Flags: needinfo?(jmuizelaar)
I'm seeing similar problems in Safari.  Also, when I add the third stock, the first and third show up, but not the second one (for the 1 day and 5 day ranges.)  I also don't see the problem when, say, comparing Apple and Google stocks.

Have we reached out to Yahoo on this?
Flags: needinfo?(milan)
Flags: needinfo?(jmuizelaar)
Whiteboard: [gfx-noted]
Karl, I have seen you looking at sitecompat issues. Milan suggested we get in touch with Yahoo on this issue as this might not be something we can fix. Would you be able to help with that? Thanks!
Flags: needinfo?(kdubost)
Flags: needinfo?(jmuizelaar)
Hi,

I don't see the problem either on Safari or Chrome (OS X) - the charts display alright.
I see the problem on Firefox 45.0.1 irrespective of the stock chosen - including for AAPL and GOOG.

Thanks,
Ravi
I can't reproduce the issue on Safari or Chrome neither. We already have a clear regression window; should be fixed on the Firefox side.
Flags: needinfo?(bas)
Attached image two-plots-on-yahoo.png
When going to 
http://finance.yahoo.com/echarts?s=WIPRO.NS+Interactive#{%22range%22:%221d%22,%22allowChartStacking%22:true}

I'm getting redirected to
http://finance.yahoo.com/echarts?s=WIPRO.NS+Interactive#{%22allowChartStacking%22:true}

fwiw I do not have the issue in Firefox Nightly 48.0a1 (2016-03-27)
Flags: needinfo?(kdubost)
Ah yes interesting behavior is different for 1d 5d.

ok. Also when changing the settings in visualization aka the gear.
Agreed with Kohei, we need first to understand where the issue is coming from.

Either Firefox and fix it
Or Yahoo! and know why it doesn't work anymore.

This requires more analysis.
(In reply to Ravi S from comment #11)
> I don't see the problem either on Safari or Chrome (OS X) - the charts
> display alright.
> I see the problem on Firefox 45.0.1 irrespective of the stock chosen -
> including for AAPL and GOOG.

(In reply to Kohei Yoshino [:kohei] from comment #12)
> I can't reproduce the issue on Safari or Chrome neither. We already have a
> clear regression window; should be fixed on the Firefox side.

I attached the screen grabs from the bug reproducing in Safari.  1 and 5 day are missing Google stock graph, 1 month has it.
That's not related to this bug. You see GOOGL N/A in that range. HCLTECH.NS in comment 0 works.
Fair enough.  It does feel like a push group regression, mentioned in comment 4.  Some observations:

* Adding another stock (e.g., Google) after HCLTECH.NS makes that one show up (nothing to show for Google)
* Panning the whole graph, especially to the left, I can see the HCLTECH.NS graph show up, in parts, usually clipped. 

(N/A is not necessarily an indicator, because it shows where the mouse cursor is, so if you don't have the data for the whole range, you'll see N/A when you'd doing a clip box type of screen grab.)
Assignee: nobody → bas
I see this on Windows as well.
OS: Mac OS X → All
Another interesting pattern with this bug.

If I display 1d, then 5d Indeed I receive a Chart unavailable most of the time.
Then I go to 1 month, and 3 months, etc.
Then go down from 3m to 1m, then 1m to 5d, then 5d to 1d, and all charts are being displayed fine.

I even managed to have the chart not displayed for 1m. We still need to understand why it's happening and more or less randomly.

Each time we change the view, we get a series of 3 HTTP requests.

* http://geo.yahoo.com/p?s=2022773886&t=rbK4LU6Mjxzvic4p,0.0373193842344699&_I=&_AO=0&_NOL=0&_R=&_P=3.21%05_pl%031%04A_v%033.21%04test%03DDCONTROL%04_bt%03rapid%04A_sid%03mmnGZQPMKP7Z0XVY%04_w%03finance.yahoo.com%2Fecharts%3Fs%3DWIPRO.NS%2BInteractive%23%7B%2522range%2522%3A%25221d%2522%2C%2522allowChartStacking%2522%3Atrue%7D%04version%03td%20app%04site%03mobile-web-quotes%04_ts%031459212240%04sec%03chart-controls-range%04slk%031mo%04_E%03click

* http://y.analytics.yahoo.com/fpc.pl?_cb=rtx1TJHhJLlEQbey&.ys=2022773886&a=1000911397279&b=WIPRO.NS%20Interactive%20Stock%20Chart%20%7C%20Yahoo!%20Inc.%20Stock%20-%20Yahoo!%20Finance&d=Tue%20Mar%2029%202016%2009%3A44%3A00%20GMT%2B0900%20(JST)&f=http%3A%2F%2Ffinance.yahoo.com%2Fecharts%3Fs%3DWIPRO.NS%2BInteractive%23%7B%2522range%2522%3A%25223mo%2522%2C%2522allowChartStacking%2522%3Atrue%7D&j=1920x1200&k=24&t=1459212240&l=true&c=interactive-chart&dpid=1846106777&x=12&fpc=ZXVaI2ZY%7C%7C&cf14=DDCONTROL&cf18=chart-controls-range&cf19=1mo&cf42=mobile-web-quotes&ca=1&resp=img

* https://finance-yql.media.yahoo.com/v7/finance/chart/WIPRO.NS?period2=1459212240&period1=1449189840&interval=1d&indicators=quote&includeTimestamps=true&includePrePost=true&comparisons=HCLTECH.NS&events=div%7Csplit%7Cearn&corsDomain=finance.yahoo.com


There's a recurring error in the console.log but again not systematic.

NS_ERROR_FAILURE: 
e.exports.init/c</<()
combo:54
m/<()
combo:53
forEach()
self-hosted
m()
combo:53
e.exports.init/c<()
combo:54
p/<()
combo:53
Kr/<()


Exceptions are raised on 
http://l.yimg.com/zz/combo?os/mit/td/td-applet-charts-0.3.1/td-charts-model/td-charts-model-min.js&os/mit/td/td-applet-charts-0.3.1/td-charts-image-model/td-charts-image-model-min.js&os/mit/td/td-applet-charts-0.3.1/td-charts-virgo-view/td-charts-virgo-view-min.js

 !c && t.xLabelText && (c = t.xLabelText.node().getBBox().width, p = c / 2, d = Math.max(0, Math.min(e.width - c - l, o - p - f))),


When the graph is working, every is working fine continuously.
(In reply to Milan Sreckovic [:milan] from comment #24)
> Fair enough.  It does feel like a push group regression, mentioned in
> comment 4.  Some observations:
> 
> * Adding another stock (e.g., Google) after HCLTECH.NS makes that one show
> up (nothing to show for Google)
> * Panning the whole graph, especially to the left, I can see the HCLTECH.NS
> graph show up, in parts, usually clipped. 
> 
> (N/A is not necessarily an indicator, because it shows where the mouse
> cursor is, so if you don't have the data for the whole range, you'll see N/A
> when you'd doing a clip box type of screen grab.)

You're seeing these inconsistencies because you're comparing stocks from two different exchanges with different market durations and times. Please use any two stocks from the same exchange to reproduce the original issue.
Yes, I can reproduce the original issue on multiple platforms, and the panning problem I mentioned is probably in support of this being push group related issue and a fallout from bug 1210560.
Moving to Jeff, Bas has too many tracked bugs.
Assignee: bas → jmuizelaar
Anthony, this bug is a good sanity test case to add to our list.  The panning as well.  I know it's broken now, but to track and test in the future.
Flags: needinfo?(anthony.s.hughes)
Attached file Reduced test cast (obsolete) —
It looks like the problem comes from having a clip-path and a mask
(In reply to Jeff Muizelaar [:jrmuizel] from comment #31)
> Created attachment 8735983 [details]
> Reduced test cast
> 
> It looks like the problem comes from having a clip-path and a mask

This testcase doesn't do anything for me -- all I get is a blank page. What steps are needed to get this to work?
Flags: needinfo?(anthony.s.hughes)
(In reply to Anthony Hughes (:ashughes) [GFX][QA][Mentor] from comment #32)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #31)
> > Created attachment 8735983 [details]
> > Reduced test cast
> > 
> > It looks like the problem comes from having a clip-path and a mask
> 
> This testcase doesn't do anything for me -- all I get is a blank page. What
> steps are needed to get this to work?

Fixing the bug :)

The testcase displays blank in broken Firefox. If you use one from before bug 1210560 it will show a graph.
Flags: needinfo?(jmuizelaar)
Summary: Yahoo finance chart comparison "overlays" not displayed properly → Yahoo finance chart comparison "overlays" not displayed properly because of bad interaction with scaled clip and mask combo
This switches us to using FillRect for combining Mask and Clip path surfaces. Doing so allows us to avoid allocating a new source and making a copy. More importantly it uses the whole matrix instead of just the offset. This was the cause of this bug. aExtraMaskTransform has a scale component that we were ignoring.
Flags: needinfo?(bas)
Attachment #8736057 - Flags: review?(jwatt)
Attachment #8736057 - Flags: review?(bas)
I'll turn this into a reftest
Attachment #8735983 - Attachment is obsolete: true
Have you tested this patch with DrawTargetCG? I'm worried that bug 1231643 might bite you there.
(In reply to Markus Stange [:mstange] from comment #36)
> Have you tested this patch with DrawTargetCG? I'm worried that bug 1231643
> might bite you there.

It seems to be biting me.
Attachment #8736057 - Flags: review?(bas) → review+
Since it got mentioned today, wanted to record it - the original test case does assert in debug.
In writing a test case for this it looks like I've discovered an additional bug.
This version adds a test and corrects some of the problems of the previous version. 

1. DrawTargetCG doesn't properly support OPERATOR_IN with A8 surfaces and is not easily fixed (cairo-quartz falls back to pixman). We use a Cairo draw target in that case.

2. We were not properly handling the area outside of the mask image. The test I added caught this.
Attachment #8736057 - Attachment is obsolete: true
Attachment #8736057 - Flags: review?(jwatt)
Attachment #8736568 - Flags: review?(jwatt)
Attachment #8736568 - Flags: review?(bas)
There's apparently something broken with D2D as well...
Comment on attachment 8736568 [details] [diff] [review]
Change how we combine clip and path masks v2

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

::: layout/svg/nsSVGClipPathFrame.cpp
@@ +124,5 @@
>    }
>  
> +  RefPtr<DrawTarget> maskDT;
> +  // The CoreGraphics backend doesn't support IN with A8 surfaces
> +  if (aReferenceDT.GetBackendType() == BackendType::COREGRAPHICS)

nit: {}
Attachment #8736568 - Flags: review?(bas) → review+
Attachment #8736568 - Attachment is obsolete: true
Attachment #8736568 - Flags: review?(jwatt)
Attachment #8736729 - Flags: review?(jwatt)
Attachment #8736729 - Flags: review?(bas)
Attachment #8736729 - Flags: review?(bas) → review+
Comment on attachment 8736729 [details] [diff] [review]
Change how we combine clip and path masks v3

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

::: layout/svg/nsSVGClipPathFrame.cpp
@@ +245,1 @@
>    }

There is some serious transformation complexity involved here :)

Some random notes, for what they're worth.

mat starts as reference context transform * -devSpaceClipExtents; it gets inverted a few lines above.

Above, where we do:
mat.Invert();
we could move the code from further down right after it:
*aMaskTransform = ToMatrix(mat);

Then this:

maskDT->MaskSurface(SurfacePattern(currentMask, ExtendMode::CLAMP, aExtraMasksTransform.Inverse() * ToMatrix(mat))...

becomes this:

maskDT->MaskSurface(SurfacePattern(currentMask, ExtendMode::CLAMP, aExtraMasksTransform.Inverse() *(*aMaskTransform))...

And we could add a comment that reminds us that:

aExtraMasksTransform.Inverse() *(*aMaskTransform)

is the same as:

(original mat, pre Invert call * aExtraMasksTransform ).Inverse()

in other words, the matrix we're using in the MaskSurface call is:

(aReferenceContext.CurrentMatrix() *
 gfxMatrix::Translation(-devSpaceClipExtents.TopLeft()) *
 aExtraMasksTransform).Inverse()
https://hg.mozilla.org/mozilla-central/rev/afb08f003648
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
This bug is a driver of the potential 45.0.2 release. The patch needs to be uplifted.

https://wiki.mozilla.org/Firefox/Channels/Meetings/2016-04-05
Flags: needinfo?(jmuizelaar)
Comment on attachment 8736729 [details] [diff] [review]
Change how we combine clip and path masks v3

Approval Request Comment
[Feature/regressing bug #]: 1210560
[User impact if declined]: Some SVG scenarios are not properly drawn.  The example we have is from a high traffic site.
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: Low.  The case where the results are different should only be those that asserted before (and were wrong.)
[String/UUID change made/needed]:
Flags: needinfo?(jmuizelaar)
Attachment #8736729 - Flags: review?(jwatt)
Attachment #8736729 - Flags: approval-mozilla-release?
Attachment #8736729 - Flags: approval-mozilla-beta?
Attachment #8736729 - Flags: approval-mozilla-aurora?
Comment on attachment 8736729 [details] [diff] [review]
Change how we combine clip and path masks v3

Let's take it for 45.0.2
Attachment #8736729 - Flags: approval-mozilla-release?
Attachment #8736729 - Flags: approval-mozilla-release+
Attachment #8736729 - Flags: approval-mozilla-esr45+
Attachment #8736729 - Flags: approval-mozilla-beta?
Attachment #8736729 - Flags: approval-mozilla-beta+
Attachment #8736729 - Flags: approval-mozilla-aurora?
Attachment #8736729 - Flags: approval-mozilla-aurora+
This does not apply cleanly to aurora, so likely doesn't apply to any of the other branches this was approved for. Can we get rebased patches?
Flags: needinfo?(jmuizelaar)
Flags: needinfo?(jmuizelaar)
Attached patch Untested beta patch (obsolete) — Splinter Review
The beta patch fails tests. I expect it will be broken on release as well.
Attachment #8738715 - Attachment is obsolete: true
It's worth noting that this fix might require part1 of bug 1251431 to work on Windows with D2D
Jeff mentioned on IRC that he will be running exhaustive tests on this patch which may take a couple of hours. We both agree that landing this patch without running it through all tests is risky. Let's wait for the test run to finish before deciding to uplift the rebased patch or whether we need more changes as mentioned in comment 56. Sylvestre, fyi.
Flags: needinfo?(sledru)
It looks like this should be ok to land on release on top of part 1 from bug 1251431
Well, not sure I want to do that in a dot release...  :/
Flags: needinfo?(sledru)
Hey Jeff, we uplifted this patch to Beta, Aurora and ESR45. Should we uplift part1 from bug 1251431 to those branches as well to ensure we have a complete and fully working fix?
Flags: needinfo?(jmuizelaar)
(In reply to Ritu Kothari (:ritu) from comment #62)
> Hey Jeff, we uplifted this patch to Beta, Aurora and ESR45. Should we uplift
> part1 from bug 1251431 to those branches as well to ensure we have a
> complete and fully working fix?

Yes.
Flags: needinfo?(jmuizelaar)
Not taking it to 45. The bug 1251431 didn't get enough user coverage to take the risk.
[Tracking Requested - why for this release]: I'm assuming this needs to be 46+ now since it's not landing for an ESR45 dot release (looks like the approval flags need some updating too).

Also, my understanding is that bug 1251431 also needs ESR45 approval before this can land for 45.1?
Flags: needinfo?(sledru)
I am not even sure I want to take it in ESR... Bug 1251431 didn't have time to back properly...
Flags: needinfo?(sledru)
Flags: qe-verify+
Reproduced the initial issue with Firefox 45.0.2.

Confirming the fix across platforms (Win 10x64, Ubuntu 14.04x64 and Mac OS X 10.11 using:
- Latest 48.0a1 Nightly, buildID 20160413030239
- Latest 47.0a2 Aurora, buildID 20160413004016
- Firefox 46.0b10, buildID 20160411042519.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Jeff or Milan, do you think this is worth a release note? If so can you suggest wording? Thanks.
Flags: needinfo?(milan)
Flags: needinfo?(jmuizelaar)
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #70)
> Jeff or Milan, do you think this is worth a release note? If so can you
> suggest wording? Thanks.

Sure. "Fix an issue where scaled SVG that used a clip and a mask was not rendered properly (bug 1258650)"
Flags: needinfo?(jmuizelaar)
Flags: needinfo?(milan)
You need to log in before you can comment on or make changes to this bug.