Closed Bug 1343879 Opened 8 years ago Closed 8 years ago

Reload-like thing happen by opening find bar on https://tc39.github.io/ecma262/

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox51 --- unaffected
firefox52 --- unaffected
firefox-esr52 --- unaffected
firefox53 --- unaffected
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: arai, Assigned: bzbarsky)

References

Details

(Keywords: regression)

Attachments

(2 files)

screen capture of the issue
732.52 KB, video/webm
Details
Be consistent about the parent style context the document-level anonymous content container should get: it should get no parent style context
2.64 KB, patch
bholley
: review+
gchang
: approval-mozilla-aurora+
Details | Diff | Splinter Review
Steps to reproduce: 1. run nightly 54.0a1 (2017-03-02) (64-bit) with clean profile, on OSX 2. open about:config and change dom.w3c_touch_events.enabled to 1 3. open https://tc39.github.io/ecma262/ 4. hit Cmd+F to open find bar Actual result: Find bar appears, and then the page looks like reloaded, but no network traffic what happens is: 1. page becomes blank (maybe from side bar, and then main content) 2. page is shown again, but with less content (page length is short, and it's cut at some point) 3. sometimes it shows full page after 1~2 seconds, sometimes remaining parts are not shown Expected result: Find bar appears, and nothing else happens This doesn't happen with dom.w3c_touch_events.enabled==0
regression range: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=846d96d2edb4&tochange=08cec1c29418 maybe a regression from bug 1331322 ?
Blocks: 1331322
Component: DOM → CSS Parsing and Computation
Flags: needinfo?(bobbyholley)
Keywords: regression
here's the screen capture.
similar issue happened before in bug 1326478
See Also: → 1326478
Sounds like we're doing a full relayout (and maybe restyle?) or something, and maybe interruptible reflow is getting involved too?
bz said he'd poke at this. Thanks Boris!
Flags: needinfo?(bobbyholley) → needinfo?(bzbarsky)
So this page uses viewport units. Therefore any viewport resize will cause a full restyle of the whole page. When we do this, we do end up reframing a <div> whose parent is <html>. This div has class="moz-custom-content-container". It of course tests true for IsNativeAnonymous(). This is why dom.w3c_touch_events.enabled matters: with that set, PresShell::AccessibleCaretEnabled returns true (because "layout.accessiblecaret.enabled_on_touch" defaults to true), so nsCanvasFrame::CreateAnonymousContent inits the accessible caret stuff, which injects some divs via nsIDocument::InsertAnonymousContent, which removes the "hidden" attribute on the canvas frame's mCustomContentContainer, which makes it actually get a frame. Anyway, so now we're trying to reframe the custom content container. We land in nsCSSFrameConstructor::RecreateFramesForContent, discover that "frame->GetStateBits() & NS_FRAME_ANONYMOUSCONTENTCREATOR_CONTENT" is true, walk up using nsLayoutUtils::GetParentOrPlaceholderFor until we find the nearest ancestor (in this case the nsCanvasFrame), and reframe its content (in this case the root element). OK, so why are we reframing this thing at all? When we initially call nsCSSFrameConstructor::ConstructAnonymousContentForCanvas, we end up in nsCSSFrameConstructor::AddFCItemsForAnonymousContent with aFrame being the nsCanvasFrame. We leave it as the style parent; see the explicit inheritFrame->GetType() == nsGkAtoms::canvasFrame check. We resolve style for the element and the placeholder, using the nsCanvasFrame's style as the parent. Then we go to do the restyle. When restyling the placeholder for the absolutely positioned custom element container, we call nsPlaceholderFrame::GetParentStyleContext. That calls CorrectStyleParentFrame, which tries to walk out of anon boxes and hence returns null. Then we end up with a null parent style context for the new placeholder style, it goes through the "this is a root" fixup and gets blockified display, we now have a display change from inline to block on the placeholder, so we reframe it. Why did this use to work? Before bug 1331322, nsCSSFrameConstructor::AddFCItemsForAnonymousContent used to call ResolveStyleContext(), passing the nsCanvasFrame as aFrame. This called nsFrame::CorrectStyleParentFrame, which returned null, so we consistently used null as the parent style context here. So the real issue is that we added that "if (inheritFrame->GetType() == nsGkAtoms::canvasFrame)" special-case in AddFCItemsForAnonymousContent that doesn't match how style reresolution works.
Flags: needinfo?(bzbarsky)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Comment on attachment 8844565 [details] [diff] [review] Be consistent about the parent style context the document-level anonymous content container should get: it should get no parent style context Review of attachment 8844565 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for fixing this! A test would be nice if you can think of one.
Attachment #8844565 - Flags: review?(bobbyholley) → review+
I haven't thought of one yet. :(
Pushed by bzbarsky@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/7bf4e5ea128d Be consistent about the parent style context the document-level anonymous content container should get: it should get no parent style context. r=bholley
had to back this out from m-i because this is causing merge conflict when trying to merge m-i to m-c in warning: conflicts while merging layout/base/nsCSSFrameConstructor.cpp! i think this is this because of the change https://hg.mozilla.org/mozilla-central/rev/afd58f4674d1 that landed before from autoland on m-c
Flags: needinfo?(bzbarsky)
Backout by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/3b8c0a06aedd Backed out changeset 7bf4e5ea128d for causing merge conflicts
No, it's merge conflicts from bug 1343937. <sigh>. Bobby, I really wish we'd stick to inbound for what are effectively Gecko-only changes with no servo/stylo dependencies of any sort. :( Especially because pulling autoland and developing on top of that is such a minefield.
Flags: needinfo?(bzbarsky)
Pushed by bzbarsky@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/8c3640c241d8 Be consistent about the parent style context the document-level anonymous content container should get: it should get no parent style context. r=bholley
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #13) > No, it's merge conflicts from bug 1343937. <sigh>. Bobby, I really wish > we'd stick to inbound for what are effectively Gecko-only changes with no > servo/stylo dependencies of any sort. Why is that better than just using autoland for everything? The main annoyance of autoland is that you can't push directly, but we have the bits to solve that. In general, this problem is pretty orthogonal to stylo. We have two integration repos, and the repo people land to is mostly related to their workflow and not related to what part of the code they work on. > :( Especially because pulling > autoland and developing on top of that is such a minefield. That problem is the same whether we use inbound or autoland, right? Statistically, autoland has less bustage...
> That problem is the same whether we use inbound or autoland, right? Only if you posit direct pushes to autoland, bypassing mozreview. I had been assuming we should use that only in extreme situations.
Comment on attachment 8844565 [details] [diff] [review] Be consistent about the parent style context the document-level anonymous content container should get: it should get no parent style context Approval Request Comment [Feature/Bug causing the regression]: bug 1331322 [User impact if declined]: Very poor performance on some sites [Is this code covered by automated tests?]: No. [Has the fix been verified in Nightly?]: Not yet. [Needs manual test from QE? If yes, steps to reproduce]: Yes. See comment 0. [List of other uplifts needed for the feature/fix]: Bug 1343937 [Is the change risky?]: Not very. [Why is the change risky/not risky?]: Mostly restores previous behavior. But this code is a bit tricky.... [String changes made/needed]: None
Attachment #8844565 - Flags: approval-mozilla-aurora?
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #16) > > That problem is the same whether we use inbound or autoland, right? > > Only if you posit direct pushes to autoland, bypassing mozreview. I had > been assuming we should use that only in extreme situations. I guess my point is that the request of "please use inbound for non-stylo" stuff doesn't really make sense organization-wide, and if we're just talking about the specific ergonomics of our group of people that happens to be hacking on the same code, we're all pushing most of our stuff to autoland and have the bits to do it painlessly. I don't have a strong preference here, it's just not really clear what the model is that would tell me what to push to which tree, and absent a model I just default to the same one all the time.
https://hg.mozilla.org/mozilla-central/rev/8c3640c241d8
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Blocks: 1342026
Hi :arai, could you help verify if this issue was fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(arai.unmht)
yes, confirmed the fix on 55.0a1 (2017-03-09) (64-bit) thanks!
Flags: needinfo?(arai.unmht)
Comment on attachment 8844565 [details] [diff] [review] Be consistent about the parent style context the document-level anonymous content container should get: it should get no parent style context Fix a regression and was verified. Aurora54+.
Attachment #8844565 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: