Closed Bug 828312 Opened 12 years ago Closed 8 years ago

HTML file with Javascript completely hangs Firefox 18.0

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Version:
18 Branch
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla30
Tracking Flags:
Tracking Status
firefox18 - wontfix
firefox19 + wontfix
firefox20 + wontfix
firefox21 + wontfix

People

(Reporter: robert, Assigned: dbaron)

References

(Blocks 2 open bugs)

Details

(Keywords: hang, regression, Whiteboard: [leave open])

Attachments

(16 files, 12 obsolete files)

lift.pli.html - auto generated file that causes crash (OK up to FF 17.0.1)
1015.56 KB, text/html
Details
windbg log (nightly21.0a1)
110.21 KB, text/plain
Details
, patch 1: Remove unneeded assignment to nonInheritedHints, which is not needed because we will do difference computation on the style context in this codepath anyway.
1.53 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
patch 4: Remove unused nsIPresShell::StyleChangeReflow.
3.91 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
simplified testcase for regression from patch 3
904 bytes, text/html
Details
patch 2: Make list renumbering code set NS_FRAME_HAS_DIRTY_CHILDREN correctly on intermediate blocks and inlines, and NS_FRAME_IS_DIRTY correctly on the bullets.
3.43 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
patch 3: Wait to mark lines dirty until we're in reflow, to avoid O(N^2) behavior as a result of looking for lines.
7.37 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
patch 4.5: Pass the same aMinChange to the top level of a ReResolveStyleContext that we would for the recursive calls, rather than including previous continutaions in aMinChange.
2.53 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
patch 5: Don't handle UpdateCursor more than once per round of style change processing, since it's global.
1.88 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
patch 6: Make early transform handling only check continuations.
3.66 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
patch 7: Use more typical loop structure and don't mutate |frame| in UpdateOverflow hint handling.
1.52 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
patch 8: Make handling of UpdateEffects hint check continuations.
1.53 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
patch 10: Add assertions to check that handling of nsChangeHint_ChildrenOnlyTransform doesn't need to check continuations.
2.10 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
patch 11: Don't generate change hints for restyling of later continuations, since the handling of the change hints from the first continuation is required to do the necessary work.
10.17 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
patch 9b: Make handling of RecomputePosition hint check continuations.
3.84 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
patch 9a: Make sticky positioning handle block-in-inline splits correctly.
20.79 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/20100101 Firefox/18.0 Build ID: 20130104151925 Steps to reproduce: I try to open a generated HTML file, that has opened OK in Firefox since at least version 3.x Actual results: Firefox hangs and it's CPU usage immediately goes to 100% (or in my case 12.5% on an 8-core CPU). This happens both in safe mode, and with all extensions enabled. When FF is already open, the unscaled text is on occasion visible, when FF is not open and the file is double-clicked, FF will open, and the tab will show "Connecting", until FF is terminated with the Taskmanager. System: W7-64 Pro (fully up to date) FF Info: Application Basics Name Firefox Version 18.0 User Agent Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/20100101 Firefox/18.0 Build Configuration about:buildconfig Extensions Name Version Enabled ID Adblock Plus 2.2.1 true {d10d0bf8-f5b5-c8b4-a8b2-2b9879e08c5d} British English Dictionary 1.19.1 true en-GB@dictionaries.addons.mozilla.org CheckPlaces 2.6.2 true checkplaces@andyhalford.com Classic Toolbar Buttons 1.1.8 true CSTBB@NArisT2_Noia4dev DownloadHelper 4.9.12 true {b9db16a4-6edc-47ec-a1f4-b86292ed211d} Firebug 1.11.1 true firebug@software.joehewitt.com Go Parent Folder 2.7 true goParentFolder@alice Html Validator 0.9.5.1 true {3b56bcc7-54e5-44a2-9b44-66c3ef58c13e} Lietuvių kalbos rašybos tikrinimo žodynas 1.2.1.20101118 true lt@dictionaries.addons.mozilla.org NewTabURL 2.2.3 true newtaburl@sogame.cat Nightly Tester Tools 3.3 true {8620c15f-30dc-4dba-a131-7c5d20cf4a29} NoScript 2.6.4.2 true {73a6fe31-595d-460b-a920-fcc0f8843232} NoSquint 2.1.6 true nosquint@urandom.ca Reload Tab On Double-Click 1.2.1 true {aede9b05-c23c-479b-a90e-9146ed62d377} Show Parent Folder 2.0 true showParentFolder@alice Status-4-Evar 2012.07.08.17 true status4evar@caligonstudios.com Woordenboek Nederlands 3.1.0 true nl-NL@dictionaries.addons.mozilla.org Xmarks 4.1.3 true foxmarks@kei.com Important Modified Preferences Name Value accessibility.typeaheadfind.flashBar 0 browser.cache.disk.capacity 1048576 browser.cache.disk.smart_size.first_run false browser.cache.disk.smart_size_cached_value 1048576 browser.display.background_color #C0C0C0 browser.display.use_system_colors true browser.history_expire_days.mirror 5 browser.history_expire_days_min 10 browser.places.createdSmartBookmarks true browser.places.importBookmarksHTML false browser.places.importDefaults false browser.places.leftPaneFolderId -1 browser.places.migratePostDataAnnotations false browser.places.smartBookmarksVersion 4 browser.places.updateRecentTagsUri false browser.privatebrowsing.dont_prompt_on_enter true browser.search.openintab true browser.startup.homepage https://www.google.com/ browser.startup.homepage_override.buildID 20130104151925 browser.startup.homepage_override.mstone 18.0 browser.tabs.insertRelatedAfterCurrent false browser.tabs.loadFolderAndReplace false browser.tabs.onTop false browser.tabs.tabMinWidth 50 browser.tabs.warnOnClose false browser.urlbar.filter.javascript false browser.urlbar.hideGoButton true browser.urlbar.maxRichResults 0 browser.urlbar.trimURLs false browser.zoom.siteSpecific false dom.disable_window_move_resize true dom.event.contextmenu.enabled false extensions.checkCompatibility true extensions.checkCompatibility.3.6 true extensions.checkCompatibility.3.6b true extensions.checkCompatibility.3.6p true extensions.checkCompatibility.3.6pre true extensions.checkCompatibility.3.7a true extensions.checkCompatibility.4.0 true extensions.checkCompatibility.4.0b true extensions.checkCompatibility.4.0p true extensions.checkCompatibility.4.0pre true extensions.checkCompatibility.4.2 true extensions.checkCompatibility.4.2a true extensions.checkCompatibility.4.2a1 true extensions.checkCompatibility.4.2a1pre true extensions.checkCompatibility.4.2b true extensions.checkCompatibility.5.0 true extensions.checkCompatibility.5.0a true extensions.checkCompatibility.5.0b true extensions.checkCompatibility.6.0 true extensions.checkCompatibility.6.0a true extensions.checkCompatibility.6.0b true extensions.checkCompatibility.7.0 true extensions.checkCompatibility.7.0a true extensions.checkCompatibility.7.0b true extensions.checkCompatibility.nightly true extensions.lastAppVersion 18.0 font.internaluseonly.changed false gfx.direct3d.prefer_10_1 true mousewheel.withcontrolkey.action 3 network.cookie.blockFutureCookies false network.cookie.prefsMigrated true network.http.max-connections 64 network.http.max-persistent-connections-per-server 8 network.http.proxy.version 1.0 network.image.imageBehavior 0 network.protocol-handler.warn-external.itms false places.database.lastMaintenance 1357521768 places.history.expiration.transient_current_max_pages 104858 places.history.expiration.transient_optimal_database_size 167772160 places.last_vacuum 1295669622 plugin.disable_full_page_plugin_for_types video/mpeg plugin.expose_full_path true privacy.clearOnShutdown.cookies false privacy.clearOnShutdown.downloads false privacy.clearOnShutdown.sessions false privacy.cpd.cookies false privacy.cpd.formdata false privacy.cpd.sessions false privacy.donottrackheader.enabled true privacy.sanitize.migrateFx3Prefs true privacy.sanitize.promptOnSanitize false privacy.sanitize.sanitizeOnShutdown true privacy.sanitize.timeSpan 0 security.enable_tls false security.warn_viewing_mixed false Graphics Adapter Description NVIDIA GeForce GT 520 Adapter Drivers nvd3dumx,nvwgf2umx,nvwgf2umx nvd3dum,nvwgf2um,nvwgf2um Adapter RAM 1023 ClearType Parameters Gamma: 2200 Pixel Structure: RGB ClearType Level: 50 Enhanced Contrast: 50 Device ID 0x1040 Direct2D Enabled true DirectWrite Enabled true (6.1.7601.17789) Driver Date 10-2-2012 Driver Version 9.18.13.697 GPU #2 Active false GPU Accelerated Windows 1/1 Direct3D 10 Vendor ID 0x10de WebGL Renderer Google Inc. -- ANGLE (NVIDIA GeForce GT 520 ) AzureCanvasBackend direct2d AzureContentBackend direct2d AzureFallbackCanvasBackend cairo JavaScript Incremental GC true Accessibility Activated false Prevent Accessibility 0 Library Versions Expected minimum version Version in use NSPR 4.9.4 4.9.4 NSS 3.14.1.0 Basic ECC 3.14.1.0 Basic ECC NSSSMIME 3.14.1.0 Basic ECC 3.14.1.0 Basic ECC NSSSSL 3.14.1.0 Basic ECC 3.14.1.0 Basic ECC NSSUTIL 3.14.1.0 3.14.1.0 Expected results: The file should open normally, although there may be some delay due to the Javascript resizing and generation of sequence numbers.
Severity: normal → major
Attachment #699785 - Attachment mime type: text/plain → text/html
Confirmed, FF18 hangs (25% of my CPU) and is not able to respond during the page is loading, must be killed. In FF17-, Firefox hangs just 3 or 4 sec, but is able to display the page (and stays smooth). Rgression range: m-c good=2012-09-07 bad=2012-09-08 http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=36427d4b2cf6&tochange=1d4fc0c60063
Status: UNCONFIRMED → NEW
status-firefox18: --- → affected
tracking-firefox18: --- → ?
tracking-firefox19: --- → ?
tracking-firefox20: --- → ?
tracking-firefox21: --- → ?
Ever confirmed: true
Keywords: regression, regressionwindow-wanted
I got the same regression range as result and will bisect now.
Severity: major → critical
Keywords: hang
I can reproduce the problem. http://hg.mozilla.org/mozilla-central/rev/7bce868864bf Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20130109 Firefox/21.0 ID:20130109030942 Regression window(cached m-i) Good: http://hg.mozilla.org/integration/mozilla-inbound/rev/8c83c4ea731a Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/18.0 Firefox/18.0 ID:20120907100120 Bad: http://hg.mozilla.org/integration/mozilla-inbound/rev/e6396a6c27e3 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/18.0 Firefox/18.0 ID:20120907101516 Pushlog: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=8c83c4ea731a&tochange=e6396a6c27e3
Blocks: 779968
Component: Untriaged → Style System (CSS)
Keywords: regressionwindow-wanted
Product: Firefox → Core
In local build Last Good: 309d87857ce0 First Bad: a080c1f6350d
dbaron - what's your general feel on the severity of this bug? Unless we expect this testcase to be commonplace in the wild (unclear), I don't think we need to track for release.
Assignee: nobody → dbaron
We have not seen duplicate reports of this issue, so wontfixing for FF18 and waiting till FF19 to first fix.
status-firefox18: affected → wontfix
status-firefox19: --- → affected
status-firefox20: --- → affected
status-firefox21: --- → affected
tracking-firefox18: ? → -
tracking-firefox19: ? → +
tracking-firefox20: ? → +
tracking-firefox21: ? → +
I've pinged dbaron in email to see if we're lining up a fix, or whether this is a non-urgent issue.
So so far I've found that the bit of nsFrameManager::ReResolveStyleContext that does: // We don't know what changes the previous continuation had, so // assume the worst. nonInheritedHints = nsChangeHint_Hints_NotHandledForDescendants; is, I think, bogus. But removing it doesn't fix the problem. At this point I'm going to bisect which changeset actually caused the problem. Also, it doesn't seem like an actual hang, just something really really slow; it looks like we're just generating >11000 reflow hints, presumably across continuations, and then doing essentially a full-document reflow for each one.
> At this point I'm going to bisect which changeset actually caused the problem. Alice0775 did that already; see comment 4. Looks like it's http://hg.mozilla.org/mozilla-central/rev/a080c1f6350d Do we end up with a bunch of ib splits here or something, landing us in the providerFrame != aFrame->GetParent() case?
(In reply to Boris Zbarsky (:bz) from comment #9) > > At this point I'm going to bisect which changeset actually caused the problem. > > Alice0775 did that already; see comment 4. Looks like it's > http://hg.mozilla.org/mozilla-central/rev/a080c1f6350d > > Do we end up with a bunch of ib splits here or something, landing us in the > providerFrame != aFrame->GetParent() case? Indeed. Commenting out this bit of that patch: 2.132 + if (providerFrame != aFrame->GetParent()) { 2.133 + // We don't actually know what the parent style context's 2.134 + // non-inherited hints were, so assume the worst. 2.135 + aParentFrameHintsNotHandledForDescendants = 2.136 + nsChangeHint_Hints_NotHandledForDescendants; 2.137 + } makes the page load again.
(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #10) > (In reply to Boris Zbarsky (:bz) from comment #9) > > > At this point I'm going to bisect which changeset actually caused the problem. > > > > Alice0775 did that already; see comment 4. Looks like it's > > http://hg.mozilla.org/mozilla-central/rev/a080c1f6350d > > > > Do we end up with a bunch of ib splits here or something, landing us in the > > providerFrame != aFrame->GetParent() case? > > Indeed. Commenting out this bit of that patch: > 2.132 + if (providerFrame != aFrame->GetParent()) { > 2.133 + // We don't actually know what the parent style context's > 2.134 + // non-inherited hints were, so assume the worst. > 2.135 + aParentFrameHintsNotHandledForDescendants = > 2.136 + nsChangeHint_Hints_NotHandledForDescendants; > 2.137 + } > makes the page load again. David if you have the regressing code what's the next step here? Is this something we should still be tracking? Do we have any idea on how many users this hang might be affecting? It looks like a minimal test case might be possible from your discovery in comment 10.
Flags: needinfo?(dbaron)
I started to work on a bunch of patches a few weeks ago; it's not trivial. I'll try to remember what the state is, but I got stuck on something.
Flags: needinfo?(dbaron)
Flags: needinfo?(dbaron)
Adding needinfo on :dbaron to help with next steps here. Do you recommend untracking this at this time ?
So I realized that there's another option to fixing this other than fixing the actual regression, which is to coalesce the reflows that happen for processing a single style change list. This is probably something we should be doing anyway.
Flags: needinfo?(dbaron)
Er, never mind, we already do that. The actual problem here is that nsBlockFrame::ChildIsDirty is O(N) in the number of children, so marking all the children dirty is O(N^2).
I tend to think that trying to maintain "line is dirty" state prior to reflow is probably a bad idea. We should just mark the frames dirty in the normal way. Then when a block's reflow starts, if the block has NS_FRAME_HAS_DIRTY_CHILDREN and not NS_FRAME_IS_DIRTY, we should do a pass over the lines to mark any lines with dirty children as dirty, with MarkLineDirty.
The model for processing of style changes means that when a frame with continuations has style changes, we process the style change for every continuation. While it may well be worth switching this to a different model in which we only process the style change for the first continuation and then apply it to all the continuations, that's not the model we currently use, and we don't need to loop over continuations. That said, this code was added in bug 154892 and modified in bug 502288. TODO: Investigate those bugs more, and how we ended up in the situation we're in.
Attachment #738007 - Flags: review?(bzbarsky)
The model for processing of style changes means that when a frame with continuations has style changes, we process the style change for every continuation. While it may well be worth switching this to a different model in which we only process the style change for the first continuation and then apply it to all the continuations, that's not the model we currently use, and we don't need to loop over continuations. TODO: Investigate more how we ended up in the situation we're in.
Attachment #738008 - Flags: review?(bzbarsky)
Comment on attachment 738003 [details] [diff] [review] , patch 1: Remove unneeded assignment to nonInheritedHints, which is not needed because we will do difference computation on the style context in this codepath anyway. What happens if copyFromContinuation and then newContext == oldContext?
Comment on attachment 738004 [details] [diff] [review] patch 2: Return whether all lines were marked dirty from PrepareResizeReflow. r=me
Attachment #738004 - Flags: review?(bzbarsky) → review+
Comment on attachment 738005 [details] [diff] [review] patch 3: Wait to mark lines dirty until we're in reflow, to avoid O(N^2) behavior as a result of looking for lines. >+ if (lineFrame->GetStateBits() & >+ (NS_FRAME_IS_DIRTY | NS_FRAME_HAS_DIRTY_CHILDREN)) { Perhaps: if (NS_SUBTREE_DIRTY(lineFrame)) { ? >+#define NS_BLOCK_LOOK_FOR_DIRTY_FRAMES NS_FRAME_STATE_BIT(61) Why not 60? r=me
Attachment #738005 - Flags: review?(bzbarsky) → review+
Comment on attachment 738006 [details] [diff] [review] patch 4: Remove unused nsIPresShell::StyleChangeReflow. r=me
Attachment #738006 - Flags: review?(bzbarsky) → review+
Comment on attachment 738007 [details] [diff] [review] patch 5: Don't loop over continuations in nsCSSFrameConstructor::StyleChangeReflow. Hmm. I guess this is right. r=me
Attachment #738007 - Flags: review?(bzbarsky) → review+
Comment on attachment 738008 [details] [diff] [review] patch 6: Don't loop over continuations in DoApplyRenderingChangeToTree. >+ } while ((outOfFlowFrame = nsLayoutUtils:: >+ GetNextContinuationOrSpecialSibling(outOfFlowFrame))); Out-of-flows are always block-level, so should never have special siblings, right? >+ // FIXME: FIX INDENT IF THIS WORKS So I thought about this some more, and it's not clear to me that the premise here is true. What exactly applies the hint to all continuations? In nsFrameManager::ComputeStyleChangeFor we pass the accumulated change hint as aMinChange into ReResolveStyleContext. That's a bit weird, since this is the only callsite where aMinChange is not set to the _parent_ hint but rather to the _sibling_ hint (and hence shouldn't really have the NS_HintsNotHandledForDescendantsIn subtracted from it). In any case, we don't add things to the change list if they're already in aMinChange, so in fact nothing makes sure the change is processed for all continuations other than the loop here...
Attachment #738008 - Flags: review?(bzbarsky) → review-
Comment on attachment 738007 [details] [diff] [review] patch 5: Don't loop over continuations in nsCSSFrameConstructor::StyleChangeReflow. r- for the same reason, but I agree that this means we do extra work in all sorts of cases due to the subtraction issue. :(
Attachment #738007 - Flags: review+ → review-
(In reply to Boris Zbarsky (:bz) from comment #25) > Perhaps: > > if (NS_SUBTREE_DIRTY(lineFrame)) { > > ? Yes. I forgot about that macro. > >+#define NS_BLOCK_LOOK_FOR_DIRTY_FRAMES NS_FRAME_STATE_BIT(61) > > Why not 60? I think the convention has been to use the private-space bits at the top starting from 63 and going down (to make it easier to change the boundary if we need to). I'll look further into the major issue later. (I need to refresh my ReResolveStyleContext memory.) Another option might be to switch whole-hog in the other direction, i.e., make it so that we don't add change hints for continuations, and we *require* that all change hint processing apply to all same-pseudo-type continuations.
Comment on attachment 738003 [details] [diff] [review] , patch 1: Remove unneeded assignment to nonInheritedHints, which is not needed because we will do difference computation on the style context in this codepath anyway. r- per comment 23, I think.
Attachment #738003 - Flags: review?(bzbarsky) → review-
Marking this wontfix for Fx21 as this does not look ready in time for release and we are going to build with our second last beta tomorrow. Please request approval for aurora(Fx22) asap if we are close to resolving the issue.
status-firefox21: affected → wontfix
(It's fine without the changes to nsBlockFrame::ChildIsDirty, and even fine with the additions there but not the removals. Doing some of the removal is fine as long as the aLine->MarkDirty() call inside MarkLineDirty happens.)
TODO: This causes various layout problems on facebook, flickr, etc. (It's fine without the changes to nsBlockFrame::ChildIsDirty, and even fine with the additions there but not the removals. Doing some of the removal is fine as long as the aLine->MarkDirty() call inside MarkLineDirty happens.)
TODO: This causes various layout problems on facebook, flickr, etc. (It's fine without the changes to nsBlockFrame::ChildIsDirty, and even fine with the additions there but not the removals. Doing some of the removal is fine as long as the aLine->MarkDirty() call inside MarkLineDirty happens.)
Attachment #747290 - Flags: review?(bzbarsky)
(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #35) > TODO: This causes various layout problems on facebook, flickr, etc. > (It's fine without the changes to nsBlockFrame::ChildIsDirty, and even > fine with the additions there but not the removals. Doing some of the > removal is fine as long as the aLine->MarkDirty() call inside > MarkLineDirty happens.) Oops, I forgot to remove this comment; this was fixed by the bit about looking at the ancestor-or-self of the placeholder of the float that's a child of the block.
Comment on attachment 747290 [details] [diff] [review] patch 3: Wait to mark lines dirty until we're in reflow, to avoid O(N^2) behavior as a result of looking for lines. Actually, never mind, still a reftest failure and a crashtest failure.
Attachment #747290 - Flags: review?(bzbarsky)
Revised patches, though it wouldn't surprise me if the tests discover something else that triggers the new assertion I added: https://tbpl.mozilla.org/?tree=Try&rev=e5ac20f65f69
The change to mark NS_FRAME_IS_DIRTY on the bullets fixes a failure in layout/reftests/bugs/418574-1.html exposed by patch 3.
Attachment #747577 - Flags: review?(bzbarsky)
New try run, with one more null-check and one crashtest manifest assertion count reduction: https://tbpl.mozilla.org/?tree=Try&rev=6c49edac69fd
The assertion count reduction from 12 to 8 in layout/generic/crashtests/798020-1.html is the removal of the 4 ###!!! ASSERTION: math on NS_UNCONSTRAINEDSIZE: 'NS_UNCONSTRAINEDSIZE != aState.mReflowState.mComputedBorderPadding.left && NS_UNCONSTRAINEDSIZE != aState.mReflowState.ComputedWidth()' in nsBlockFrame::PrepareResizeReflow by avoiding calling nsBlockFrame::PrepareResizeReflow.
Attachment #747747 - Flags: review?(bzbarsky)
Attachment #747578 - Attachment is obsolete: true
Attachment #747578 - Flags: review?(bzbarsky)
Looks all green now, finally.
(In reply to Boris Zbarsky (:bz) from comment #28) > In nsFrameManager::ComputeStyleChangeFor we pass the accumulated change hint > as aMinChange into ReResolveStyleContext. That's a bit weird, since this is > the only callsite where aMinChange is not set to the _parent_ hint but > rather to the _sibling_ hint (and hence shouldn't really have the > NS_HintsNotHandledForDescendantsIn subtracted from it). Interesting. That's weird, and it's inconsistent with the way we'll handle continuations that aren't at the top level of what we're re-resolving (which I'd think is by far the more common case). I think we should just fix that. > In any case, we don't add things to the change list if they're already in > aMinChange, so in fact nothing makes sure the change is processed for all > continuations other than the loop here... In this unusual case, where the continuations are continuations of the top-level frame getting the style change, indeed. Then again, I think in the long term it would be better to go in the opposite direction, and make continuations not post change hints at all -- though the profiles I've seen make me think it wouldn't be that big a difference since all we'd gain is the CalcDifference time and the overhead around change hint processing, and not anything in nsRuleNode::Compute*Data. Maybe it's not that hard, but I also don't feel like tackling it now. I suppose I should make a followup bug for it, though.
> I think we should just fix that. Seems reasonable to me. Followup makes sense.
And the full patch series with the patch 4.5 fixing that seems green: https://tbpl.mozilla.org/?tree=Try&rev=ffd64c7ae35f
The model for processing of style changes means that when a frame with continuations has style changes, we process the style change for every continuation. While it may well be worth switching this to a different model in which we only process the style change for the first continuation and then apply it to all the continuations, that's not the model we currently use, and we don't need to loop over continuations.
Attachment #747786 - Flags: review?(bzbarsky)
The model for processing of style changes means that when a frame with continuations has style changes, we process the style change for every continuation. While it may well be worth switching this to a different model in which we only process the style change for the first continuation and then apply it to all the continuations, that's not the model we currently use, and we don't need to loop over continuations.
Attachment #747787 - Flags: review?(bzbarsky)
Comment on attachment 738003 [details] [diff] [review] , patch 1: Remove unneeded assignment to nonInheritedHints, which is not needed because we will do difference computation on the style context in this codepath anyway. (In reply to Boris Zbarsky (:bz) from comment #23) > What happens if copyFromContinuation and then newContext == oldContext? I guess I don't follow the question. nonInheritedHints is for change hints on this style context that don't apply automatically to child style contexts, and therefore require us to check for them on child style contexts. If newContext == oldContext, then nonInheritedHints should be 0 because we had no change hints, just as for all the other ways we get the new style context other than copying it from the prev continuation. So re-requesting review.
Attachment #738003 - Flags: review- → review?(bzbarsky)
And another try run with all patches (including 1 and 7, which the previous didn't have): https://tbpl.mozilla.org/?tree=Try&rev=b82bd75824db
> If newContext == oldContext, then nonInheritedHints should be 0 because we had no change > hints What about cases when we had explicit change hints already in the restyle tracker, even though we still have the same style context? I guess that can't happen for later continuations, though. Can we assert if we ever end up on this codepath but pass a nonzero parent hint to our kids?
(In reply to Boris Zbarsky (:bz) from comment #54) > > If newContext == oldContext, then nonInheritedHints should be 0 because we had no change > > hints > > What about cases when we had explicit change hints already in the restyle > tracker, even though we still have the same style context? I guess that > can't happen for later continuations, though. Hmmm. That seems like a problem with patches 5, 6, and 7, though I don't see what the concern about patch 1 is. > Can we assert if we ever end up on this codepath but pass a nonzero parent > hint to our kids? "this codepath" being "newContext == oldContext"? I suppose so, though it almost seems silly to bother. (Also, at some point I might want to improve the cases where we fall back to nsChangeHint_Hints_NotHandledForDescendants because we don't have the parent's data anymore, which could make that assertion harder to keep around.)
> "this codepath" being "newContext == oldContext" Well, in general that can pass a nonzero hint if we had a hint in the restyle tracker, no? I meant the "newContext == oldContext and were not the first continuation" case....
Comment on attachment 738003 [details] [diff] [review] , patch 1: Remove unneeded assignment to nonInheritedHints, which is not needed because we will do difference computation on the style context in this codepath anyway. r=me
Attachment #738003 - Flags: review?(bzbarsky) → review+
Comment on attachment 747577 [details] [diff] [review] patch 2: Make list renumbering code set NS_FRAME_HAS_DIRTY_CHILDREN correctly on intermediate blocks and inlines, and NS_FRAME_IS_DIRTY correctly on the bullets. r=me
Attachment #747577 - Flags: review?(bzbarsky) → review+
Comment on attachment 747747 [details] [diff] [review] patch 3: Wait to mark lines dirty until we're in reflow, to avoid O(N^2) behavior as a result of looking for lines. >+ // and, and mark it as NS_FRAME_HAS_DIRTY_CHILDREN too, so that we s/and, and/and/ >+ nsIFrame *parentFC = >+ placeholderPathFC->GetParent()->GetFirstContinuation(); >+ if (parentFC == thisFC) { Can we optimize by checking placeholderPathFC->GetParent()->mContent == mContent before we get the first-continuation of placeholderPathFC->GetParent(), which might entail walking along a long inline continuation chain... In fact, why do we need to walk to placeholderPath->GetFirstContinuation() to start with? Seems like we could walk up till we get to something with the right mContent and _then_ worry about first continuations and maybe walking up more. r=me
Attachment #747747 - Flags: review?(bzbarsky) → review+
Comment on attachment 747785 [details] [diff] [review] patch 4.5: Pass the same aMinChange to the top level of a ReResolveStyleContext that we would for the recursive calls, rather than including previous continutaions in aMinChange. r=me
Attachment #747785 - Flags: review?(bzbarsky) → review+
Comment on attachment 747786 [details] [diff] [review] patch 5: Don't loop over continuations in nsCSSFrameConstructor::StyleChangeReflow. r=me
Attachment #747786 - Flags: review?(bzbarsky) → review+
Comment on attachment 747787 [details] [diff] [review] patch 6: Don't loop over continuations in DoApplyRenderingChangeToTree. r=me
Attachment #747787 - Flags: review?(bzbarsky) → review+
Comment on attachment 747788 [details] [diff] [review] patch 7: Don't loop over continuations when handling nsChangeHint_UpdateOverflow. r=me
Attachment #747788 - Flags: review?(bzbarsky) → review+
And bustage fix for patch 3 (warnings-as-errors failure from addressing comment 59): http://hg.mozilla.org/integration/mozilla-inbound/rev/603fbfa2f12e
Depends on: 871338
Here's a try run on a patch series that moves to the model where hints apply across all continuations (but not special siblings). https://tbpl.mozilla.org/?tree=Try&rev=231d73463d18
So the try run triggered a single assertion failure. What I suspect the problem is (though I haven't verified it) is that the change I made in nsFrameManager::ReResolveStyleContext no longer avoids reresolving style on the descendants of a later continuation of a content node that is going to get reframed. I think this is relatively easy to fix if I use a global frame state bit (we're almost out, I fear); I'm not sure how else to fix it.
Actually, I realized on the way home that that's not true. The problem is failure to propagate non-inherited hints correctly to descendants because we haven't computed the style change for the parent. And both problems can be fixed by changing the order we walk the frames to reach continuations through their prior continuations (including when we walk their children).
Just upgraded to FF 22.0, the bug is still present, almost six months after reporting it. What was wrong with the FF 17 code that worked flawlessly?
It was too slow in other cases; see bug 779968.
> It was too slow in other cases; see bug 779968. I don't remember how long it took FF 17 with the attached test case, (yes, more than just a few seconds) but "too slow" is relative compared to "it completely hangs the browser"... Oh well, I'll sit back and wait patiently.
(In reply to Robert Prins from comment #72) > > It was too slow in other cases; see bug 779968. > I don't remember how long it took FF 17 with the attached test case, (yes, > more than just a few seconds) but "too slow" is relative compared to "it > completely hangs the browser"... Though it's also a difference between too slow in a relatively common case vs. hangs the browser in an extremely strange case that only one user has reported. In any case, sorry for managing to drop this in the midst of some other stuff; I'll try to finish it up soon.
This is the primary reason why I temporarily shut off javascript by going to options and unchecking the box in Content, Enable Javascript. For those who do not know, javascript is what give the web stupid web tricks, like opening a new window when you click the close box so that you can not leave a website that you are not interested in. Bug 851702, now closed to comment by mere mortals and illy advised implemented in Release 23.0 needs to be reversed immediately. We can not tell our users what they want. Some people categorically shut off javascript, some, like me, shut it off when it hangs up the browser. The same is true of shutting off image loading. If someone is on a dialup connection or are nearing their ISP download limit for the month they can shut off images and reduce by billions the number of bytes downloaded. That was bug 851701, also implemented in 23, and which also needs to be reversed immediately. Only morons write web pages that require images or javascript to be viewed or used. There is absolutely no reason to cater to them by removing it from the browser options.
(This is part of the patch stack making change hints apply across all continuations and block-in-inline siblings. In this case, however, the change hint only needs to apply once, globally.)
Attachment #806304 - Flags: review?(bzbarsky)
This is part of the patch stack making change hints apply across all continuations and block-in-inline siblings.
Attachment #806305 - Flags: review?(bzbarsky)
This is part of the patch stack making change hints apply across all continuations and block-in-inline siblings.
Attachment #806307 - Flags: review?(bzbarsky)
This is part of the patch stack making change hints apply across all continuations and block-in-inline siblings.
Attachment #806308 - Flags: review?(bzbarsky)
This is part of the patch stack making change hints apply across all continuations and block-in-inline siblings.
Attachment #806309 - Flags: review?(bzbarsky)
This depends on bug 898333 in order to avoid causing: TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/dom/tests/mochitest/chrome/test_focused_link_scroll.xul | Assertion count 1 is greater than expected range 0-0 assertions. due to the assertion: ###!!! ASSERTION: Shouldn't be trying to restyle non-elements directly: '!aContent || aContent->IsElement()', file ../../../layout/base/nsStyleChangeList.cpp, line 62 The assertion count change in layout/generic/crashtests/571995.xhtml is expected because it changes us from having 7 of: ###!!! ASSERTION: Shouldn't be trying to restyle non-elements directly: '!aContent || aContent->IsElement()', file ../../../layout/base/nsStyleChangeList.cpp, line 62 with the stack: mozilla::ElementRestyler::CaptureChange(nsStyleContext*, nsStyleContext*, nsChangeHint) [layout/base/nsChangeHint.h:191] mozilla::ElementRestyler::RestyleSelf(nsRestyleHint) [layout/base/RestyleManager.cpp:2304] to only having one. This is expected since this patch changes RestyleSelf to only call CaptureChange for the first continuation or block-in-inline sibling.
Attachment #806310 - Flags: review?(bzbarsky)
Comment on attachment 806304 [details] [diff] [review] patch 5: Don't handle UpdateCursor more than once per round of style change processing, since it's global. r=me
Attachment #806304 - Flags: review?(bzbarsky) → review+
Comment on attachment 806305 [details] [diff] [review] patch 6: Make early transform handling only check continuations. r=me
Attachment #806305 - Flags: review?(bzbarsky) → review+
Comment on attachment 806306 [details] [diff] [review] patch 7: Use more typical loop structure and don't mutate |frame| in UpdateOverflow hint handling. r=me
Attachment #806306 - Flags: review?(bzbarsky) → review+
Comment on attachment 806307 [details] [diff] [review] patch 8: Make handling of UpdateEffects hint check continuations. r=me
Attachment #806307 - Flags: review?(bzbarsky) → review+
Comment on attachment 806308 [details] [diff] [review] patch 9: Make handling of RecomputePosition hint check continuations. r=me
Attachment #806308 - Flags: review?(bzbarsky) → review+
Comment on attachment 806309 [details] [diff] [review] patch 10: Add assertions to check that handling of nsChangeHint_ChildrenOnlyTransform doesn't need to check continuations. r=me
Attachment #806309 - Flags: review?(bzbarsky) → review+
Comment on attachment 806310 [details] [diff] [review] patch 11: Don't generate change hints for restyling of later continuations, since the handling of the change hints from the first continuation is required to do the necessary work. r=me
Attachment #806310 - Flags: review?(bzbarsky) → review+
Patch 9 needs to be rewritten post-bug 904197.
This assumes that the specification for how position:sticky behaves for block-in-inline splits matches the specification for position:relative, in other words, matches http://www.w3.org/TR/CSS21/visuren.html#anonymous-block-level . It's also necessary for patch 9b since the new rule for handling of style change hints is that a style change hint applies to all continuations and all block-in-inline siblings ("special siblings"). The change in StickyScrollContainer::GetScrollRanges is really the fix for bug 918994, but adjusted for the change here to use block-in-inline siblings ("special siblings") in addition to continuations.
Attachment #809553 - Flags: review?(dholbert)
This is part of the patch stack making change hints apply across all continuations and block-in-inline siblings.
Attachment #809554 - Flags: review?(bzbarsky)
And I need to file a followup bug on the FIXME comments in patch 9a, blocking the bug on enabling position:sticky.
Comment on attachment 809554 [details] [diff] [review] patch 9b: Make handling of RecomputePosition hint check continuations. r=me
Attachment #809554 - Flags: review?(bzbarsky) → review+
https://tbpl.mozilla.org/?tree=Try&rev=9352d652be11 with two fixes: (1) add back the s/aFrame/cont/ inside the loop that got lost during rebasing (2) adjust the test in patch 9b to use 20px Ahem rather than 16px, since Ahem is best used in multiples of 5px so that both ascent (0.8em) and descent (0.2em) are integer pixels
(In reply to David Baron [:dbaron] (needinfo? me) from comment #97) > https://tbpl.mozilla.org/?tree=Try&rev=9352d652be11 with two fixes: > > (1) add back the s/aFrame/cont/ inside the loop that got lost during > rebasing in patch 9b > (2) adjust the test in patch 9b to use 20px Ahem rather than 16px, since > Ahem is best used in multiples of 5px so that both ascent (0.8em) and > descent (0.2em) are integer pixels er, I meant in patch 9a for this one
This assumes that the specification for how position:sticky behaves for block-in-inline splits matches the specification for position:relative, in other words, matches http://www.w3.org/TR/CSS21/visuren.html#anonymous-block-level . It's also necessary for patch 9b since the new rule for handling of style change hints is that a style change hint applies to all continuations and all block-in-inline siblings ("special siblings"). The change in StickyScrollContainer::GetScrollRanges is really the fix for bug 918994, but adjusted for the change here to use block-in-inline siblings ("special siblings") in addition to continuations.
Attachment #809634 - Flags: review?(dholbert)
Attachment #809553 - Attachment is obsolete: true
Attachment #809553 - Flags: review?(dholbert)
Comment on attachment 809634 [details] [diff] [review] patch 9a: Make sticky positioning handle block-in-inline splits correctly. Quoting block-in-inline-[123].html: >+ #sticky { >+ display: inline; >+ position: sticky; [...] >+ <div id="sticky"> >+ before block >+ <div>in block</div> It might be clearer to use <span id="sticky"> instead of <div>, so that it's clearer that this is a block-in-inline chunk. (without needing to read the <style> block to find that the div is actually display:inline) r=me regardless, though.
Attachment #809634 - Flags: review?(dholbert) → review+
I don't like putting <div> inside <span> because then there's some risk of parse-time fixup confusing the test results. (Not sure if it's still true, but I got into that habit, and if it's a test we're going to contribute to the W3C, it seems worth sticking to.)
OK, fair enough.
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c577bd421ea (needed fuzzy-if for OS X 10.8; I'm not going to worry about it right now)
And fuzzy on android in a different way (at the edge of the characters after a space): https://hg.mozilla.org/integration/mozilla-inbound/rev/e64ed43f90ad
Does this still need to be [leave open]? And did these patches fix bug 918994?
Flags: needinfo?(dbaron)
Yes, I believe this was fixed when that last set of patches landed (i.e., for Firefox 27). Seems like I can't set that milestone anymore, though.
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: needinfo?(dbaron)
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Depends on: 1346454
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: