Closed Bug 1880928 Opened 3 months ago Closed 3 months ago

Open https://nodejs.org/docs/latest-v20.x/api/os.html#ostmpdir isn't rendered initially

Categories

(Core :: Layout, defect, P2)

Product:
Core
Component:
Layout
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
125 Branch
Tracking Flags:
Tracking Status
firefox-esr115 --- unaffected
firefox123 --- disabled
firefox124 + disabled
firefox125 + verified

People

(Reporter: hiro, Assigned: fredw)

References

(Regression,
URL
)

Details

(Keywords: regression)

Attachments

(6 files, 5 obsolete files)

1880928-reduced.html
319 bytes, text/html
Details
Bug 1880928 - Add tests for content-visibility update with contain-intrinsic-size. r=cathiechen
48 bytes, text/x-phabricator-request
Details | Review
Bug 1880928 - Make ResizeObserver::mCallback a ResizeObserverCallback again. r=Oriol
48 bytes, text/x-phabricator-request
Details | Review
Bug 1880928 - Remove last remembered size observer, handle it in Document::DetermineProximityToViewportAndNotifyResizeObservers. r=emilio,#layout
48 bytes, text/x-phabricator-request
Details | Review
Bug 1880928 - Update some "last remembered sizes" before determining proximity to the viewport. r=emilio,#layout
48 bytes, text/x-phabricator-request
Details | Review
Bug 1880928 - Disable content-visibility.
48 bytes, text/x-phabricator-request
phab-bot
: approval-mozilla-beta+
Details | Review

STR;

  1. Open https://nodejs.org/docs/latest-v20.x/api/os.html#ostmpdir in a new tab

The contents are not fully rendered properly. If you do scroll or select some text there, it will reveal the contents.

:fredw, since you are the author of the regressor, bug 1807253, could you take a look?

For more information, please visit BugBot documentation.

Flags: needinfo?(fwang)
Flags: needinfo?(fwang)
Flags: needinfo?(fwang)

[Tracking Requested - why for this release]: rendering regression from feature shipping in 124

Severity: -- → S2
status-firefox123: --- → disabled
tracking-firefox124: --- → ?
tracking-firefox125: --- → ?
Priority: -- → P2
Attached file Minimized testcase (obsolete) —

I can reproduce with a trunk build. Attached is a testcase minimized with lithium. Bug disappears if content-visibility flag is turn off.

So per the spec these content-visibility: auto elements should only turn out size containment when they "skip their contents". In that they would use the specified contain-intrinsic-height (rather than sizing as if empty).

I'm assuming the #target thing should reveal div "2" making it take a small size. Then as a consequence of it, the same would happen for its siblings making everything visible.

However, checking the inspector when the bug happens, 1-2-3 actually have a non-zero small height (like if it they would skip their content?) and 4 has 5000px height (like if it would skip its content?). But the page is scrolled to the bottom with div 4 taking all the viewport size, hence showing no text.

Flags: needinfo?(fwang)

The bug is marked as tracked for firefox124 (beta) and tracked for firefox125 (nightly). However, the bug still isn't assigned.

:fgriffith, could you please find an assignee for this tracked bug? Given that it is a regression and we know the cause, we could also simply backout the regressor. If you disagree with the tracking decision, please talk with the release managers.

For more information, please visit BugBot documentation.

Flags: needinfo?(fgriffith)

So do we have a way forward for a fix, are we considering backing out the regressor, or do we turn off content-vis by default until we have a solution?

Flags: needinfo?(fwang)
Flags: needinfo?(fgriffith)
Flags: needinfo?(emilio)

(In reply to Frank Griffith Jr from comment #6)

So do we have a way forward for a fix, are we considering backing out the regressor, or do we turn off content-vis by default until we have a solution?

I'm not clear what's the regressor is but backing out the multiple content-visibility patches will likely be tricky, so turning off will likely be the easiest work around.

(In reply to BugBot [:suhaib / :marco/ :calixte] from comment #5)

The bug is marked as tracked for firefox124 (beta) and tracked for firefox125 (nightly). However, the bug still isn't assigned.

Assigning myself, as I plan to investigate more.

Assignee: nobody → fwang
Flags: needinfo?(fwang)

I just checked a bit what's happening with attachment 9381018 [details] when the bug happens.

Placing some loggings in PresShell::ProximityToViewportResult PresShell::DetermineProximityToViewport() one can see the function is repeated indefinitely and the proximities of content-visibility: auto divs don't stabilize:

  • At one call last div is close to viewport and the others far from it.
  • At the next call last div is far from viewport and the others close to it.
  • At the next call last div is close to viewport and the others far from it.
  • etc

Checking nsRefreshDriver::Tick, we actually don't stop ticking, the reason is always eHasObservers | eNeedsToNotifyResizeObserver. We actually don't have resize observers on the page but this is happening because of our "last remember size" observer for the contain-intrinsic-height node (see backtrace below). emilio mentioned in the past we can probably get rid of that observer and handle this stuff directly when determining proximity to the viewport (see bug 1807253 comment 39) so that's probably what we should do to avoid this kind of subtle issues...

#0  0x00007f0b9f31efc1 in nsRefreshDriver::EnsureResizeObserverUpdateHappens() (this=0x7f0b8bf3ea00) at layout/base/nsRefreshDriver.h:426
#1  0x00007f0b9f2c3ecf in mozilla::dom::Document::ScheduleResizeObserversNotification() const (this=0x7f0bb35eb300) at dom/base/Document.cpp:17137
#2  0x00007f0b9f3d7e23 in mozilla::dom::ResizeObserver::Observe(mozilla::dom::Element&, mozilla::dom::ResizeObserverOptions const&) (this=0x7f0b8bff4820, aTarget=..., aOptions=...) at dom/base/ResizeObserver.cpp:317
#3  0x00007f0b9f2c1a27 in mozilla::dom::Document::ObserveForLastRememberedSize(mozilla::dom::Element&) (this=0x7f0bb35eb300, aElement=...) at dom/base/Document.cpp:16450
#4  0x00007f0ba379d8cc in nsIFrame::HandleLastRememberedSize() (this=0x7f0b9038c3a8) at layout/generic/nsIFrame.cpp:1422
#5  0x00007f0ba37ad194 in nsIFrame::UpdateIsRelevantContent(mozilla::EnumSet<mozilla::ContentRelevancyReason, unsigned char> const&) (this=0x7f0b9038c3a8, aRelevancyToUpdate=const nsIFrame::ContentRelevancy & = {...}) at layout/generic/nsIFrame.cpp:7159
#6  0x00007f0ba35dc5bc in mozilla::PresShell::DetermineProximityToViewport() (this=0x7f0b8ac04000) at layout/base/PresShell.cpp:12176
#7  0x00007f0b9f2c3f54 in mozilla::dom::Document::DetermineProximityToViewportAndNotifyResizeObservers() (this=0x7f0bb35eb300) at dom/base/Document.cpp:17168
#8  0x00007f0ba358470a in nsRefreshDriver::DetermineProximityToViewportAndNotifyResizeObservers() (this=0x7f0b8bf3ea00) at layout/base/nsRefreshDriver.cpp:2278
#9  0x00007f0ba35837a6 in nsRefreshDriver::Tick(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp, nsRefreshDriver::IsExtraTick) (this=0x7f0b8bf3ea00, aId=..., aNowTime=..., aIsExtraTick=nsRefreshDriver::IsExtraTick::No)
    at layout/base/nsRefreshDriver.cpp:2770
Attached file 1880928-reduced.html —

This reduces testcase a bit more, using scrollIntoView() to scroll to the target instead of clicking a link a forcing a reload.

Attachment #9381018 - Attachment is obsolete: true

Thanks Fred. So I posted comment 11 which is in line with what I meant bug 1807253 comment 39. I confirmed that fixes the rendering of the test-case.

The thing that's missing is for DetermineProximityToViewport (which should probably be renamed) to also clear stale remembered sizes for things that no longer have frames, which is a bit of work but hopefully straight-forward? I think PresShell can keep a list of weak refs to elements that call UnregisterContentVisibilityAutoFrame. For every element in that list which doesn't have a content visibility auto frame we just clear the last remembered size.

If you don't have the cycles to do that I'm happy to give it a poke.

Flags: needinfo?(emilio)

Thanks Emilio, I can confirm my WPT tests and all the testcases work properly with your patches.

From https://drafts.csswg.org/css-sizing-4/#last-remembered the last remembered size is updated "At the time that ResizeObserver events are determined and delivered" which is now done at step 15 of https://html.spec.whatwg.org/#event-loop-processing-model so that's indeed consistent with the suggestion to move things to DetermineProximityToViewportAndNotifyResizeObservers().

Note that the function exits early if the document has neither resize observers nor content-visibility: auto frames per https://searchfox.org/mozilla-central/rev/a8cc31504a2379bcf8ba395d2da7bb632b5521d6/layout/base/nsRefreshDriver.cpp#2266 ; which means removing the last remember size observer may cause it to be executed less often.

@Oriol: Since you worked on the last remember size stuff, do you think there is any other cases we should consider too?

Flags: needinfo?(oriol-bugzilla)

Essentially reverting D154324 since we no longer need it to accept
internal callback functions.

Sent current patchset to try server and there are many failures: https://treeherder.mozilla.org/jobs?repo=try&revision=b51383897a2af77eb67890512316c378915a48fc

For content-visibility: auto nodes that are relevant to the user,
make sure their recorded "last remembered sizes" is up-to-date so that
the proximity to the viewport can be properly determined.

This change is covered by tests added in D202403.

Attachment #9382004 - Attachment is obsolete: true
Attachment #9382282 - Attachment description: WIP: Bug 1880928 - Remove last remembered size observer, handle it in Document::DetermineProximityToViewportAndNotifyResizeObservers. → Bug 1880928 - Remove last remembered size observer, handle it in Document::DetermineProximityToViewportAndNotifyResizeObservers. r=emilio,#layout
Attachment #9382647 - Attachment description: WIP: Bug 1880928 - Update some "last remembered sizes" before determining proximity to the viewport → Bug 1880928 - Update some "last remembered sizes" before determining proximity to the viewport. r=emilio,#layout

(In reply to Frédéric Wang (:fredw) from comment #13)

Thanks Emilio, I can confirm my WPT tests and all the testcases work properly with your patches.

Same here with the latest patchset.

Note that the function exits early if the document has neither resize observers nor content-visibility: auto frames per https://searchfox.org/mozilla-central/rev/a8cc31504a2379bcf8ba395d2da7bb632b5521d6/layout/base/nsRefreshDriver.cpp#2266 ; which means removing the last remember size observer may cause it to be executed less often.

OK, so as discussed on D202424 we also need to handle last remembered size updates for other nodes, not just content-visibility: auto nodes. So D202571 removes the internal resize observer and should be more correct. D202731 contains the actual fix from Emilio's original patch: update the last remembered sizes of some content-visibility: auto nodes before determining the proximity to viewport.

(In reply to Frédéric Wang (:fredw) from comment #15)

Sent current patchset to try server and there are many failures: https://treeherder.mozilla.org/jobs?repo=try&revision=b51383897a2af77eb67890512316c378915a48fc

I'm getting better results locally with the new patch set. I sent them to try: https://treeherder.mozilla.org/jobs?repo=try&revision=4832c3a3f89c3657141a9c62412b8e6cd170d18e

Flags: needinfo?(oriol-bugzilla)
Attachment #9382282 - Attachment description: Bug 1880928 - Remove last remembered size observer, handle it in Document::DetermineProximityToViewportAndNotifyResizeObservers. r=emilio,#layout → WIP: Bug 1880928 - Remove last remembered size observer, handle it in Document::DetermineProximityToViewportAndNotifyResizeObservers. r=emilio,#layout
Attachment #9382282 - Attachment description: WIP: Bug 1880928 - Remove last remembered size observer, handle it in Document::DetermineProximityToViewportAndNotifyResizeObservers. r=emilio,#layout → Bug 1880928 - Remove last remembered size observer, handle it in Document::DetermineProximityToViewportAndNotifyResizeObservers. r=emilio,#layout
Attachment #9387219 - Attachment is obsolete: true
Attachment #9382647 - Attachment description: Bug 1880928 - Update some "last remembered sizes" before determining proximity to the viewport. r=emilio,#layout → WIP: Bug 1880928 - Update some "last remembered sizes" before determining proximity to the viewport. r=emilio,#layout

(In reply to Frédéric Wang (:fredw) from comment #8)

Checking nsRefreshDriver::Tick, we actually don't stop ticking, the reason is always eHasObservers | eNeedsToNotifyResizeObserver. We actually don't have resize observers on the page but this is happening because of our "last remember size" observer for the contain-intrinsic-height node (see backtrace below). emilio mentioned in the past we can probably get rid of that observer and handle this stuff directly when determining proximity to the viewport (see bug 1807253 comment 39) so that's probably what we should do to avoid this kind of subtle issues...

So another subtle thing is that content-visibility: auto forces contain-intrinsic-height to gain auto per https://searchfox.org/mozilla-central/rev/9cd4ea81e27db6b767f1d9bbbcf47da238dd64fa/servo/components/style/style_adjuster.rs#536 ; this is why we ever use a "last remember size" even if auto is not explicit in the reduced testcases. The original testcase at https://nodejs.org/docs/latest-v20.x/api/assets/style.css really uses an explicit auto though:

#apicontent section {
  content-visibility: auto;
  contain-intrinsic-size: 1px auto 5000px;
}

Backtrace:

(rr) rc
thread 1 hit Hardware watchpoint 2: -location mContainIntrinsicHeight

Old value = mozilla::StyleGenericContainIntrinsicSize<mozilla::StyleCSSPixelLength> {tag: mozilla::StyleGenericContainIntrinsicSize<mozilla::StyleCSSPixelLength>::Tag::AutoLength, : {length: mozilla::StyleGenericContainIntrinsicSize<mozilla::StyleCSSPixelLength>::StyleLength_Body {_0: mozilla::StyleCSSPixelLength {_0: 5000}}, auto_length: mozilla::StyleGenericContainIntrinsicSize<mozilla::StyleCSSPixelLength>::StyleAutoLength_Body {_0: mozilla::StyleCSSPixelLength {_0: 5000}}}}
New value = mozilla::StyleGenericContainIntrinsicSize<mozilla::StyleCSSPixelLength> {tag: mozilla::StyleGenericContainIntrinsicSize<mozilla::StyleCSSPixelLength>::Tag::Length, : {length: mozilla::StyleGenericContainIntrinsicSize<mozilla::StyleCSSPixelLength>::StyleLength_Body {_0: mozilla::StyleCSSPixelLength {_0: 5000}}, auto_length: mozilla::StyleGenericContainIntrinsicSize<mozilla::StyleCSSPixelLength>::StyleAutoLength_Body {_0: mozilla::StyleCSSPixelLength {_0: 5000}}}}
0x00007ff0d07d9679 in style::properties::generated::gecko::GeckoPosition::set_contain_intrinsic_height (self=0x7ff0b4387bb8, v=...) at obj-x86_64-pc-linux-gnu-release/x86_64-unknown-linux-gnu/release/build/style-f050414487bf3194/out/gecko_properties.rs:3518
3518	        self.mContainIntrinsicHeight = From::from(v);
(rr) bt
#0  0x00007ff0d07d9679 in style::properties::generated::gecko::GeckoPosition::set_contain_intrinsic_height (self=0x7ff0b4387bb8, v=...)
    at obj-x86_64-pc-linux-gnu-release/x86_64-unknown-linux-gnu/release/build/style-f050414487bf3194/out/gecko_properties.rs:3518
#1  0x00007ff0d037aa68 in style::style_adjuster::StyleAdjuster::adjust_for_contain_intrinsic_size (self=0x7fffa1a02da0) at servo/components/style/style_adjuster.rs:558
Attachment #9382647 - Attachment description: WIP: Bug 1880928 - Update some "last remembered sizes" before determining proximity to the viewport. r=emilio,#layout → Bug 1880928 - Update some "last remembered sizes" before determining proximity to the viewport. r=emilio,#layout

This is doing the same as D202731 but keeping the ResizeObserver-based
implementation of last remembered sizes.

This change is covered by tests added in D202403.

Keywords: leave-open

This reverts D198689.

Pushed by fwang@igalia.com:
https://hg.mozilla.org/integration/autoland/rev/467f563c9ea5
Add tests for content-visibility update with contain-intrinsic-size. r=cathiechen
https://hg.mozilla.org/integration/autoland/rev/3508e2bd4e3c
Remove last remembered size observer, handle it in Document::DetermineProximityToViewportAndNotifyResizeObservers. r=emilio,layout-reviewers
https://hg.mozilla.org/integration/autoland/rev/e01ba93d881f
Update some "last remembered sizes" before determining proximity to the viewport. r=emilio,layout-reviewers
https://hg.mozilla.org/integration/autoland/rev/dfad870b0c8e
Make ResizeObserver::mCallback a ResizeObserverCallback again. r=Oriol

This is doing the same as D202731 but keeping the ResizeObserver-based
implementation of last remembered sizes.

It also imports the tests from D202403.

Original Revision: https://phabricator.services.mozilla.com/D202926

Attachment #9388327 - Attachment description: WIP: Bug 1880928 - Update some "last remembered sizes" before determining proximity to the viewport. → Bug 1880928 - Update some "last remembered sizes" before determining proximity to the viewport.
Attachment #9388038 - Attachment is obsolete: true
Attachment #9388324 - Attachment description: WIP: Bug 1880928 - Disable content-visibility. → Bug 1880928 - Disable content-visibility.
Attachment #9388327 - Flags: approval-mozilla-beta?
Attachment #9388324 - Flags: approval-mozilla-beta?

@emilio: I uploaded two alternatives patches for beta: either disable the feature completely or apply a similar fix without the lastremeberedsize refactoring. Please tell me what you think.

Flags: needinfo?(emilio.alvarez96)

Uplift Approval Request

  • String changes made/needed: None
  • Explanation of risk level: This is just moving when the internal ResizeObservervation for last remembered size is broadcast before determination of "proximity to viewport" for content-visibility: auto nodes, rather than after so should have minimal behavior. It is sligthly different from the (more complex) nightly patches and has only been tested locally, so we might prefer D203093 instead.
  • Fix verified in Nightly: no
  • Risk associated with taking this patch: Medium
  • Needs manual QE test: no
  • User impact if declined: This may be causing a blank rendering for a quite likely use case on modern website (several content-visibility: auto sections with a contain-intrinsic-size to optimize performance).
  • Is Android affected?: yes
  • Steps to reproduce for manual QE testing: n/a
  • Code covered by automated testing: yes
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/44851 for changes under testing/web-platform/tests

Uplift Approval Request

  • Is Android affected?: yes
  • Code covered by automated testing: no
  • Steps to reproduce for manual QE testing: Open attachment 9381952 [details]. It should not remain blank until you start interacting with the page.
  • User impact if declined: This may be causing a blank rendering for a quite likely use case on modern website (several content-visibility: auto sections with a contain-intrinsic-size to optimize performance).
  • String changes made/needed: None
  • Explanation of risk level: This is just turning off content-visibility, which has not been shipped yet in release. We might prefer D203094 which fixes the bug and adds automated tests to verify it.
  • Risk associated with taking this patch: Low
  • Needs manual QE test: yes
  • Fix verified in Nightly: no
Flags: qe-verify+

(In reply to Frédéric Wang (:fredw) from comment #25)

@emilio: I uploaded two alternatives patches for beta: either disable the feature completely or apply a similar fix without the lastremeberedsize refactoring. Please tell me what you think.

Was supposed to be for emilio@crisal.io ; but answer is https://phabricator.services.mozilla.com/D203094#6968520

I guess that's fine and the safest option to delay content-visibility again.

Flags: needinfo?(emilio.alvarez96)
Upstream PR merged by moz-wptsync-bot
Attachment #9388324 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment on attachment 9388327 [details]
Bug 1880928 - Update some "last remembered sizes" before determining proximity to the viewport.

It was decided to disable content-visibility in 124 beta rather than uplift this patch. It remains enabled for 125.

Attachment #9388327 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Attachment #9388327 - Attachment is obsolete: true
Status: NEW → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
status-firefox125: affected → fixed
Target Milestone: --- → 125 Branch
QA Whiteboard: [qa-triaged]

Reproduced this issue on an affected Nightly build from 2024-02-19, on Windows 10.
Verified as fixed on 125.a1 (20240305094850) on the following platforms: Win 10, Win 11, Ubuntu 22 and macOS 11.

Status: RESOLVED → VERIFIED
status-firefox125: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: