1481905 - Text in dropdown under "App Functions" is not visible (was: Page not Loading after 59 version)

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P3)

Description

[manjusha.guthikonda]

Attachment: Firefox Issues Text.docx

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/68.0.3440.84 Safari/537.36

Steps to reproduce:

Please click on the below link in Firefox version higher than 58, then the page is not fully loading. 

https://ecomm2dev-sage-support.cs89.force.com/marketplace/


Actual results:

If you see at the left/banner the picklist values/text are missing but when you click on any of the app and comes back again you will be able to see all.  This is happening from Monday (08/06/2018), please see the attached screen shots.



Expected results:

Page should be loaded completely. Please see the attached screen shots.  For further information please reach out to me at [e-mail]

Comment 1

[Anne (:annevk)]

Thanks for reporting this! This looks like a layout problem, so moving it there.

Comment 2

[Anne (:annevk)]

In particular, the problem on this document appears to be that under, e.g., "App Functions" there's supposed to be a list of text items, but they do not render. You can click them however. Contrast with Chrome.

Comment 3

[manjusha.guthikonda]

(In reply to Anne (:annevk) from comment #2)
> In particular, the problem on this document appears to be that under, e.g.,
> "App Functions" there's supposed to be a list of text items, but they do not
> render. You can click them however. Contrast with Chrome.

Yes that's true, it used to work till 08/06, suddenly it stopped working in all our environments. And it is working good in version 58.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

WFM with webrender enabled, but I can see it with it disabled, so tentatively putting it on graphics. Any chance to get a regression range? (https://mozilla.github.io/mozregression/quickstart.html)

Comment 5

[manjusha.guthikonda]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #4)
> WFM with webrender enabled, but I can see it with it disabled, so
> tentatively putting it on graphics. Any chance to get a regression range?
> (https://mozilla.github.io/mozregression/quickstart.html)

Sorry I didn't get you, what do you mean by regression range?

Comment 6

[Emilio Cobos Álvarez (:emilio)]

(In reply to manjusha.guthikonda from comment #5)
> (In reply to Emilio Cobos Álvarez (:emilio) from comment #4)
> > WFM with webrender enabled, but I can see it with it disabled, so
> > tentatively putting it on graphics. Any chance to get a regression range?
> > (https://mozilla.github.io/mozregression/quickstart.html)
> 
> Sorry I didn't get you, what do you mean by regression range?

If you run that program called mozregression, you can find the particular change in Firefox that broke that website.

Comment 7

[manjusha.guthikonda]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #6)
> (In reply to manjusha.guthikonda from comment #5)
> > (In reply to Emilio Cobos Álvarez (:emilio) from comment #4)
> > > WFM with webrender enabled, but I can see it with it disabled, so
> > > tentatively putting it on graphics. Any chance to get a regression range?
> > > (https://mozilla.github.io/mozregression/quickstart.html)
> > 
> > Sorry I didn't get you, what do you mean by regression range?
> 
> If you run that program called mozregression, you can find the particular
> change in Firefox that broke that website.

I tried running it and the last I remembered was it was working well till friday(08/03), if I keep that as good I am still not seeing all values. Do you want to have a quick call/ you need anymore information for that range.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Here's the regression range mozreview came up with:

  https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=1735ff2bb23e&tochange=818f353b7aa6

It's not clear to me if it's a clipping issue, or a text drawing issue or what not. Jeff, do you know what could make this fixed in WR?

Comment 9

[Emilio Cobos Álvarez (:emilio)]

err, s/mozreview/mozregression. It's not quite a recent regression btw.

Comment 10

[Jeff Muizelaar [:jrmuizel]]

Nothing from that regression range looks that suspicious and I have no idea why it would work with WebRender. I reduced test case would be valuable.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Oh, looks like a font fallback issue, if I change the font to something that isn't AdelleSansSageLt it works...

Comment 12

[manjusha.guthikonda]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #11)
> Oh, looks like a font fallback issue, if I change the font to something that
> isn't AdelleSansSageLt it works...

But when we have AdelleSansSageLt previously it used to work.

Comment 13

[manjusha.guthikonda]

(In reply to manjusha.guthikonda from comment #12)
> (In reply to Emilio Cobos Álvarez (:emilio) from comment #11)
> > Oh, looks like a font fallback issue, if I change the font to something that
> > isn't AdelleSansSageLt it works...
> 
> But when we have AdelleSansSageLt previously it used to work and when we come from inside an app it loads the data. Yesterday we checked with version 58 it was working fine but now in 58 also it is not working.

Comment 14

[Emilio Cobos Álvarez (:emilio)]

So, locally, with the same font, it works just fine.

The issue is that when we're fetching the font from the following URL:

  https://ecomm2dev-sage-support.cs89.force.com/marketplace/resource/1529515085000/AdelleSansFontNew/adellesanssage-light.woff2

We get an empty response (or something else gets confused, or what not). So this is either a networking or a font-face loader bug somehow.

Btw, that font-face rule is repeated a bunch of times across the page, probably you want to fix that.

Comment 15

[manjusha.guthikonda]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #14)
> So, locally, with the same font, it works just fine.
> 
> The issue is that when we're fetching the font from the following URL:
> 
>  
> https://ecomm2dev-sage-support.cs89.force.com/marketplace/resource/
> 1529515085000/AdelleSansFontNew/adellesanssage-light.woff2
> 
> We get an empty response (or something else gets confused, or what not). So
> this is either a networking or a font-face loader bug somehow.
> 
> Btw, that font-face rule is repeated a bunch of times across the page,
> probably you want to fix that.

I got your point what you are saying, but I am confused/concerned is why it is working earlier and why not now. And when I come from inside an app why is the font loading. 

Btw, that font-face rule is repeated a bunch of times across the page, probably you want to fix that. - It might seem like the same page but we are loading different pages from that so it should be in that way.

Comment 16

[Jonathan Kew [:jfkthame]]

Looking at the range from comment 8, I wonder if this might be a regression from bug 998869 (but have not confirmed that).

Comment 17

[Emilio Cobos Álvarez (:emilio)]

(In reply to Jonathan Kew (:jfkthame) from comment #16)
> Looking at the range from comment 8, I wonder if this might be a regression
> from bug 998869 (but have not confirmed that).

Making this loop never execute:

  https://searchfox.org/mozilla-central/rev/0f4d50ff5211e8dd85e119ef683d04b63062ed90/layout/style/FontFaceSet.cpp#798

"fixes" it, preventing the cancellation of the load. But it looks to me like that font should've been removed from oldRecords. Let me dig a bit more.

Comment 18

[Alice0775 White]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #4)
> WFM with webrender enabled, but I can see it with it disabled, so
> tentatively putting it on graphics. Any chance to get a regression range?
> (https://mozilla.github.io/mozregression/quickstart.html)

I can reproduce the issue on Nightly63.0a1 even if WebRender is enabled.
* First time I cannot reproduce the issue at first time. But, After restart browser, I can reproduce the issue.
  So, this seems to be timing issue.
                       ============

STR in Nightly63.0a1
1. Create New profile
2. Start Firefox with the profile and Quit
3. Start Firefox again with the profile
4. Open the URL ( https://ecomm2dev-sage-support.cs89.force.com/marketplace/ ) 
  --- The page might be rendered properly.
5. Quit browser
6. Start Firefox again with the profile
  --- The fonts does not appear. The page is not rendered properly.
      And the problem persist even if restart browser


Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=c0c410aed86f&tochange=a07df88f972e


:jtd
Your bunch of old patch seems to trigger the issue, can you please look into this?

Regressed by: Bug 998869

Comment 19

[Emilio Cobos Álvarez (:emilio)]

Attachment: repro.zip

Here's something that reproduces consistently (with Ctrl + F5, and serving from that server which delays the response).

STR:

 (1) python server.py
 (2) Navigate to localhost:8000/test.html

We have multiple @font-face sources, we remove the first and cancel the load, but there are other font faces that have the same source for which we don't trigger a load afterwards.

I'm not really that familiar with the font loading code, so Jonathan, if you could take a look it'd be awesome. Otherwise I can try to get to it, just let me know.

Comment 20

[Emilio Cobos Álvarez (:emilio)]

s/localhost:8000/localhost:8001 above.

Comment 21

[manjusha.guthikonda]

If it create a difference, I removed the forcing functionality(!important) for font - family, but still we are having the same issue. I know you are all working **** this but as we are planned to go live on Aug 14th this is the only one that is blocking us, Can this be prioritized. Appreciate all your hard work on this.

Comment 22

[Emilio Cobos Álvarez (:emilio)]

(In reply to manjusha.guthikonda from comment #21)
> If it create a difference, I removed the forcing functionality(!important)
> for font - family, but still we are having the same issue. I know you are
> all working **** this but as we are planned to go live on Aug 14th this is
> the only one that is blocking us, Can this be prioritized. Appreciate all
> your hard work on this.

That makes no difference, you need to avoid removing the stylesheet that contains the @font-face rule in the first place. Even if we got it fixed on time, given this is a really old regression, we're not going to fix all affected Firefox versions.

So I'd recommend working around it avoiding to remove the sheet instead.

Comment 23

[manjusha.guthikonda]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #22)
> (In reply to manjusha.guthikonda from comment #21)
> > If it create a difference, I removed the forcing functionality(!important)
> > for font - family, but still we are having the same issue. I know you are
> > all working **** this but as we are planned to go live on Aug 14th this is
> > the only one that is blocking us, Can this be prioritized. Appreciate all
> > your hard work on this.
> 
> That makes no difference, you need to avoid removing the stylesheet that
> contains the @font-face rule in the first place. Even if we got it fixed on
> time, given this is a really old regression, we're not going to fix all
> affected Firefox versions.
> 
> So I'd recommend working around it avoiding to remove the sheet instead.

Even if you can fix it in current version that would be a lot of help for us. As the font-family is one our Sage font it is not feasible for us to avoid/remove that. But anyways I will discuss with our team

Comment 24

[Emilio Cobos Álvarez (:emilio)]

(In reply to manjusha.guthikonda from comment #23)
> Even if you can fix it in current version that would be a lot of help for
> us. As the font-family is one our Sage font it is not feasible for us to
> avoid/remove that. But anyways I will discuss with our team

You don't need to remove the font-family. You're removing a stylesheet from the page that had that @font-face rule. If you avoided doing that, the font would load normally, and it would work.

Comment 25

[manjusha.guthikonda]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #24)
> (In reply to manjusha.guthikonda from comment #23)
> > Even if you can fix it in current version that would be a lot of help for
> > us. As the font-family is one our Sage font it is not feasible for us to
> > avoid/remove that. But anyways I will discuss with our team
> 
> You don't need to remove the font-family. You're removing a stylesheet from
> the page that had that @font-face rule. If you avoided doing that, the font
> would load normally, and it would work.

I got it what you are saying but the main issue is if I remove the  @font-face rule then my page will not recognize the desired font. So we need that rule in place.

Comment 26

[Emilio Cobos Álvarez (:emilio)]

(In reply to manjusha.guthikonda from comment #25)
> I got it what you are saying but the main issue is if I remove the 
> @font-face rule then my page will not recognize the desired font. So we need
> that rule in place.

You _don't_ need to remove the @font-face rule. You _are_ removing one stylesheet that has that @font-face rule (though there are more sheets with the same font-face), which is what triggers this bug. You need not to remove it.

Comment 27

[Emilio Cobos Álvarez (:emilio)]

Attachment: When canceling a user font load, make sure to not leave mUserFontLoadState as LOADING.

Since that's what we use to kick off new loads, should they be needed.

Comment 28

[Emilio Cobos Álvarez (:emilio)]

Help turning repro.zip into a reftest or something would be appreciated, though I don't know any way of not making it network-timing dependent, so it'd have a lot of chances of becoming flacky... :(

Comment 29

[manjusha.guthikonda]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #26)
> (In reply to manjusha.guthikonda from comment #25)
> > I got it what you are saying but the main issue is if I remove the 
> > @font-face rule then my page will not recognize the desired font. So we need
> > that rule in place.
> 
> You _don't_ need to remove the @font-face rule. You _are_ removing one
> stylesheet that has that @font-face rule (though there are more sheets with
> the same font-face), which is what triggers this bug. You need not to remove
> it.

Ok, now I removed it all references and kept it only in one place, Can you check the below link and see from your side if it is loading fine every time. sometimes may be due to Catches or something it is showing as empty so I wanted you to confirm if it is looking good at th ebackend on your side.

https://mktp-sage-support.cs84.force.com/marketplace.

Comment 30

[Emilio Cobos Álvarez (:emilio)]

Yeah, looks fine now, at least on Nightly.

Comment 31

[Jonathan Kew [:jfkthame]]

Attachment: Testcase for loading subsequent @font-face source after an in-progress load is cancelled

Here's a reftest based on your reduced testcase. Not sure how reliable this will be over the long term, given the dependence on a timing issue, but it seems to work OK in my testing.

Comment 32

[Jonathan Kew [:jfkthame]]

Comment on attachment 8999197 [details]
When canceling a user font load, make sure to not leave mUserFontLoadState as LOADING.

Jonathan Kew (:jfkthame) has approved the revision.

Comment 33

[Jonathan Kew [:jfkthame]]

Try run showing that the reftest fails consistently without the patch:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5491fd7c9848ff069d577c38d11c796298f89490

And passes once your fix is applied:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=799023c951ca70f7d74ab0d412cf5fe9c97f762c

(Ignore the couple of unrelated random-oranges!)

Comment 34

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8999400 [details] [diff] [review]
Testcase for loading subsequent @font-face source after an in-progress load is cancelled

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

Nice :)

Is this sjs stuff documented somewhere?

Comment 35

[Jonathan Kew [:jfkthame]]

There's a bit of info on MDN, e.g.:
https://developer.mozilla.org/en-US/docs/Mozilla/httpd.js/HTTP_server_for_unit_tests#SJS:_server-side_scripts
https://developer.mozilla.org/en-US/docs/Mozilla/Projects/Mochitest#How_do_I_write_tests_that_check_header_values.2C_method_types.2C_etc._of_HTTP_requests.3F (about mochitest, but http reftests can also use sjs)

Or you can just cargo-cult from existing examples, like I do. ;)

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/49daf4d2dee7
When canceling a user font load, make sure to not leave mUserFontLoadState as LOADING. r=jfkthame
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b10b850fddb
Testcase for loading subsequent @font-face source after an in-progress load is cancelled. r=emilio

Comment 37

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/49daf4d2dee7
https://hg.mozilla.org/mozilla-central/rev/6b10b850fddb

Comment 38

[manjusha.guthikonda]

Thank you all for your work to resolve this issue with in the given time frame.

Comment 39

[Ryan VanderMeulen [:RyanVM]]

Is this something we should consider uplifting or can it ride the trains?

Comment 40

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8999197 [details]
When canceling a user font load, make sure to not leave mUserFontLoadState as LOADING.

Yeah, I think uplifting should be fine. Note that this request is for both changesets.

Approval Request Comment
[Feature/Bug causing the regression]: bug 998869
[User impact if declined]: Text not showing up in pages
[Is this code covered by automated tests?]: Yes (thanks Jonathan!)
[Has the fix been verified in Nightly?]: Yes (just did)
[Needs manual test from QE? If yes, steps to reproduce]: See comment 19, though I don't think it needs verification.
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: Not much I'd say
[Why is the change risky/not risky?]: Simple change resetting some state in the font cancellation code path.
[String changes made/needed]: none

Comment 41

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8999197 [details]
When canceling a user font load, make sure to not leave mUserFontLoadState as LOADING.

Fix for what looks like an edge case, looks low risk and includes a new test; let's uplift for beta 19.

Comment 42

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8999400 [details] [diff] [review]
Testcase for loading subsequent @font-face source after an in-progress load is cancelled

[Triage Comment]

Comment 43

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/releases/mozilla-beta/rev/d8ac2a1b5f6c
https://hg.mozilla.org/releases/mozilla-beta/rev/7887ffd25489