Closed Bug 828312 Opened 11 years ago Closed 7 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.)
Attachment #738004 - Attachment is obsolete: true
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)
Attachment #738005 - Attachment is obsolete: true
Attachment #747289 - Attachment is obsolete: true
(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)
Attachment #747290 - Attachment is obsolete: true
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)
Attachment #738007 - Attachment is obsolete: true
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)
Attachment #738008 - Attachment is obsolete: true
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+
Landed patches 1 to 4.5:
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6979d9274697
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/bf5fd9db9078
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/09552b219c3a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9c42b49f5cac
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b7330cb50416

I think 5-7 are bad as stated in comment 55; I think the rest of the fix is therefore the long term plan from comment 45.
OS: Windows 7 → All
Hardware: x86_64 → All
Whiteboard: [leave open]
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.
Blocks: 895810
Depends on: 898333
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)
Attachment #747786 - Attachment is obsolete: true
Attachment #747787 - Attachment is obsolete: true
Attachment #747788 - Attachment is obsolete: true
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+
Blocks: 918064
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)
Attachment #806308 - Attachment is obsolete: true
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: 7 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.