Closed Bug 1481905 Opened 2 years ago Closed 2 years ago

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

Categories

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

60 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox-esr52 --- wontfix
firefox-esr60 --- wontfix
firefox61 --- wontfix
firefox62 --- fixed
firefox63 --- fixed

People

(Reporter: manjusha.guthikonda, Assigned: emilio)

References

()

Details

(Keywords: regression)

Attachments

(4 files)

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 manjusha.guthikonda@sage.com
Thanks for reporting this! This looks like a layout problem, so moving it there.
Status: UNCONFIRMED → NEW
Component: DOM → Layout
Ever confirmed: true
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.
(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.
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)
Component: Layout → Graphics
Summary: Page not Loading after 59 version → Text in dropdown under "App Functions" is not visible (was: Page not Loading after 59 version)
(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?
(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.
(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.
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?
Flags: needinfo?(jmuizelaar)
err, s/mozreview/mozregression. It's not quite a recent regression btw.
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.
Flags: needinfo?(jmuizelaar)
Oh, looks like a font fallback issue, if I change the font to something that isn't AdelleSansSageLt it works...
(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.
(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.
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.
Component: Graphics → Networking
(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.
Looking at the range from comment 8, I wonder if this might be a regression from bug 998869 (but have not confirmed that).
(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.
Component: Networking → CSS Parsing and Computation
(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
Attached file 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.
Flags: needinfo?(jfkthame)
s/localhost:8000/localhost:8001 above.
Flags: needinfo?(emilio)
Priority: -- → P3
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.
(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.
(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
(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.
(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.
(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.
Since that's what we use to kick off new loads, should they be needed.
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... :(
Assignee: nobody → emilio
Flags: needinfo?(emilio)
Flags: needinfo?(jfkthame)
Attachment #8999197 - Flags: review?(jfkthame)
(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.
Yeah, looks fine now, at least on Nightly.
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.
Attachment #8999400 - Flags: review?(emilio)
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.
Attachment #8999197 - Flags: review+
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 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?
Attachment #8999400 - Flags: review?(emilio) → review+
Pushed by emilio@crisal.io:
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
https://hg.mozilla.org/mozilla-central/rev/49daf4d2dee7
https://hg.mozilla.org/mozilla-central/rev/6b10b850fddb
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Flags: needinfo?(jd.bugzilla)
Attachment #8999197 - Flags: review?(jfkthame)
Thank you all for your work to resolve this issue with in the given time frame.
Flags: in-testsuite+
Is this something we should consider uplifting or can it ride the trains?
Flags: needinfo?(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
Flags: needinfo?(emilio)
Attachment #8999197 - Flags: approval-mozilla-beta?
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.
Attachment #8999197 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8999400 [details] [diff] [review]
Testcase for loading subsequent @font-face source after an in-progress load is cancelled

[Triage Comment]
Attachment #8999400 - Flags: approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.