Closed Bug 496500 Opened 11 years ago Closed 10 years ago

While resizing a long page on Windows lines at the bottom do not get reflowed

Categories

(Core :: Layout, defect)

x86
Windows XP
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: tnikkel, Assigned: bzbarsky)

References

Details

Attachments

(2 files)

When resizing a windows on Windows there is always an interrupt (because HasPendingInputEvent always returns true after we've been sent WM_ENTERSIZEMOVE until we get a WM_EXITSIZEMOVE, regardless if the user actually changed the size of the window or there is any other input). nsBlockFrame does a CheckForInterrupt for every line, so once we have >= sInterruptChecksToSkip lines (approximately), no reflow will ever finish. The effect is visible in the testcase, but also in many other pages. (If you don't have the fix for bug 491700 then you won't be able to see anything during resize.)
Attached patch Possible fixSplinter Review
Timothy, can you give this a shot and see how it looks for you?
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
For what it's worth, this might not help on all pages; I'd be interested in how it does for you on your testcase, html5 single-page, and whatever page you first saw it on.
(In reply to comment #2)
> For what it's worth, this might not help on all pages; I'd be interested in how
> it does for you on your testcase, html5 single-page, and whatever page you
> first saw it on.

Fixes the testcase, does not fix http://en.wikipedia.org/wiki/United_States. http://www.whatwg.org/specs/web-apps/current-work/ I think is just too long to test as it even chokes up an opt-build that has ireflow outside of a vm (given that I don't have the latest and greatest hardware).
With further testing this patch seems to cause the hang of bug 491700 to return on http://en.wikipedia.org/wiki/United_States but with much less frequency. Changing the PostReflowEvent to run off a timer in PresShell::FrameNeedsReflow fixes the problem.
Hmm.  What's making that FrameNeedsReflow call?  We should probably take that to bug 491700.

As far as the HTML5 spec goes, I forgot to mention that you probably want to turn off JS when testing on that.  It has onresize handlers that force uninterruptible reflow, so within 2 seconds of stopping the resize, you'll get a freezeup.

As far as wikipedia goes, an interesting experiment is to run this from the url bar on that site: 

  javascript:var s = document.createElement("style"); s.appendChild(document.createTextNode("* { float: none ! important; }")); document.body.appendChild(s); void(0);

and see how it affects the behavior.  It might just be that enough stuff is impacted by floats (and hence gets marked dirty) that this fix doesn't help anything.
This might be a stupid question, but shouldn't the WM_EXITSIZEMOVE trigger a reflow to clean this up after the resize is complete even if the lines do not reflow during the resize?
(In reply to comment #6)
> This might be a stupid question, but shouldn't the WM_EXITSIZEMOVE trigger a
> reflow to clean this up after the resize is complete even if the lines do not
> reflow during the resize?

Your are correct. After the resize is over the page reflows as normal.
(In reply to comment #7)
> (In reply to comment #6)
> > This might be a stupid question, but shouldn't the WM_EXITSIZEMOVE trigger a
> > reflow to clean this up after the resize is complete even if the lines do not
> > reflow during the resize?
> 
> Your are correct. After the resize is over the page reflows as normal.

Well that was kind of the point of my question.  With just the patch form bug 491700 and not the patch form here, the lines at the bottom of the screen still never get correctly reflowed using the attached testcase.  Isn't that kind of a separate issue from the fact that they don't get reflowed during the resize?
(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > This might be a stupid question, but shouldn't the WM_EXITSIZEMOVE trigger a
> > > reflow to clean this up after the resize is complete even if the lines do not
> > > reflow during the resize?
> > 
> > Your are correct. After the resize is over the page reflows as normal.
> 
> Well that was kind of the point of my question.  With just the patch form bug
> 491700 and not the patch form here, the lines at the bottom of the screen still
> never get correctly reflowed using the attached testcase.  Isn't that kind of a
> separate issue from the fact that they don't get reflowed during the resize?

I just double checked to make sure I wasn't lying to you. With just the patch from 491700 using the testcase in this bug I get a proper reflow after resize, but not during. If that is happening to you then it seems like it would be separate and needs to be investigated.
How odd.  Although I am sure it was doing this before, I am no longer able to reproduce it. I now see the same behavior you are.  If I get it to do this again I will file a new bug with steps to reproduce.
(In reply to comment #5)
> As far as the HTML5 spec goes, I forgot to mention that you probably want to
> turn off JS when testing on that.  It has onresize handlers that force
> uninterruptible reflow, so within 2 seconds of stopping the resize, you'll get
> a freezeup.

That makes it bearable, but it still suffers from not reflowing lines while resizing (even with removing floats).

> As far as wikipedia goes, an interesting experiment is to run this from the url
> bar on that site: 
> 
>   javascript:var s = document.createElement("style");
> s.appendChild(document.createTextNode("* { float: none ! important; }"));
> document.body.appendChild(s); void(0);
> 
> and see how it affects the behavior.  It might just be that enough stuff is
> impacted by floats (and hence gets marked dirty) that this fix doesn't help
> anything.

Doesn't help.
> Doesn't help.

That's odd....  it did help over here when I tried it (in the same vm where I've seen weird results for window resizes before, so take with a grain of salt).  I have no idea why in your case that page is not working....
Perhaps it would help to try to come up with a mechanism to replace the whole maximum number of interrupts to skip logic with a mechanism to have a maximum time to skip interrupts.
If checking for times on Windows were not insanely expensive, maybe...
That said, I suppose we could check the time every N lines....

That still wouldn't help cases where it takes more than about 100ms to make progress on the page; the interrupt check should absolutely not be more rare than that.
I actually coded this up and, besides being expensive, it didn't really seem to help.
With this patch I noticed an odd behaviour. If I do a resize and drag the window to the minimum width, and then drag it wider the bottom lines do not reflow -- they stay wrapped as if the window was still that skinny. Exiting the resize of course reflows the lines as normal.
On which testcase?
Sorry. I should have mentioned on the testcase in this bug.
Oh, that probably makes sense for that situation; I bet all the lines end up dirty every reflow in that case.
Attachment #381820 - Flags: superreview?(roc)
Attachment #381820 - Flags: review?(roc)
Comment on attachment 381820 [details] [diff] [review]
Possible fix

I think this is still worth doing.
Attachment #381820 - Flags: superreview?(roc)
Attachment #381820 - Flags: superreview+
Attachment #381820 - Flags: review?(roc)
Attachment #381820 - Flags: review+
Pushed http://hg.mozilla.org/mozilla-central/rev/505e5dc1e170 yesterday.

Marking fixed, I guess; I should have spun that patch into a different bug...

I still think the timer thing is the right way to address the rest of what's happening here, but it's running into some mac issues at the moment.  And I haven't even tried it on Windows/Linux
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.