The default bug view has changed. See this FAQ.

setting innerHTML makes page jump

VERIFIED FIXED in Firefox 9

Status

()

Core
Layout
P2
major
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: Mime Čuvalo, Assigned: bz)

Tracking

({regression, verified-aurora, verified-beta})

8 Branch
mozilla10
x86
Mac OS X
regression, verified-aurora, verified-beta
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox8- affected, firefox9+ fixed, firefox10+ fixed)

Details

(Whiteboard: [qa!])

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
Created attachment 565843 [details]
testcase exhibiting issue

My site has an infinite-scroll feature that adds more content to the bottom of the page when you get there.

However, in Firefox 8 there seems to be a regression that causes the page to jump/scroll to a point higher up in the document when this happens.  I cannot reproduce it in Firefox 7.0

I've created a test case that exhibits this behavior.  Open the file, go to the bottom of the page, click button to add a <div> and it'll make the page jump upwards.
(Reporter)

Comment 1

6 years ago
Created attachment 565844 [details]
testcase exhibiting issue

blah, for some reason bugzilla detected .html as text/plain...
Attachment #565843 - Attachment is obsolete: true
Attachment #565844 - Attachment mime type: text/plain → text/html
Attachment #565843 - Attachment mime type: text/plain → text/html
Keywords: regressionwindow-wanted
Attachment #565844 - Attachment description: testcase exhibiting issue (text/html) → testcase exhibiting issue
The testcase WFM on Windows 7.
Looks like this regressed in http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=6df31af4cca6&tochange=19348341366b

Nothing in there jumps out at me, though...
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
Oh, and I'm also on Mac.
Maybe http://hg.mozilla.org/mozilla-central/rev/4053fa47daac or http://hg.mozilla.org/mozilla-central/rev/8d56702130fb ?
The former seems pretty unlikely.  The latter, maybe.  I'm not sure when I'll have a chance to bisect on builds here; if someone else can do that it would be lovely.
(Reporter)

Comment 7

6 years ago
Playing around with the testcase some more, fwiw, it seems that clicking the button to add innerHTML doesn't make the page jump some of the time.
Well, bisect says:

changeset:   73112:4053fa47daac
user:        Jeff Muizelaar <jmuizelaar@mozilla.com>
date:        Wed Jul 20 14:27:10 2011 -0400
summary:     Bug 539093. TimeStamp implementation for OS X using mach_absolute_time. r=cjones


Shows what I know!

Jeff, Chris, any idea what's up here?
Blocks: 539093
Keywords: regressionwindow-wanted
That is ... bizarre.  No ideas off the top of my head.  Maybe ...
 - if something is checking the difference between TimeStamp t2 and t1 for the purposes of animation, then with Jeff's patch timestamps taken close to each other are more likely to be different since they have much higher resolution.  If that were the case, I would expect the bug to be in mac-specific code, because on desktop-linux we've had high-res timers for a long time.
 - maybe something doesn't expect decimal points in a millisecond value?  (/me waves hands)
 - were you testing on a multi-core machine?  Is mach_absolute_time() guaranteed to be monotonic across multiple cores?
tracking-firefox8: --- → ?
Seems more likely to be related to the TimeStamp usage in layout/base/nsRefreshDriver.{h,cpp} than anything else I can think of, though I can't immediately see how.
This only happens on OS X?
(Reporter)

Comment 12

6 years ago
Couldn't reproduce on Windows 7 (on a dual-boot Mac).
(Reporter)

Comment 13

6 years ago
(In reply to Chris Jones [:cjones] [:warhammer] from comment #9)
>  - were you testing on a multi-core machine? 

In my case, yes, Intel Core 2 Duo.
It seems to me like the regression on Aurora and Beta here should we dealt with using a backout.  Marking as tracking10? too in case this is not fixed before the next migration date.
tracking-firefox10: --- → ?
tracking-firefox9: --- → ?
I've only tested on OSX, and yes on a multi-core machine.
(In reply to Boris Zbarsky (:bz) from comment #15)
> I've only tested on OSX, and yes on a multi-core machine.

I tested on the same configuration, and could reproduce very easily.
I tried changing ClockTime like so:

  static PRUint64 lastClock = 0;
  PRUint64 newClock = mach_absolute_time();
  NS_ASSERTION(newClock >= lastClock, "How did that happen?");
  lastClock = newClock;
  return (lastClock / 1000000) * 1000000;

and the bug remains.  The assertion does not trigger.  I believe that eliminates all the guesses from comment 9.  :(
(In reply to Boris Zbarsky (:bz) from comment #17)
> I tried changing ClockTime like so:
> 
>   static PRUint64 lastClock = 0;
>   PRUint64 newClock = mach_absolute_time();
>   NS_ASSERTION(newClock >= lastClock, "How did that happen?");
>   lastClock = newClock;
>   return (lastClock / 1000000) * 1000000;
> 
> and the bug remains.  The assertion does not trigger.  I believe that
> eliminates all the guesses from comment 9.  :(

Not completely, since you didn't use a lock to protect lastClock.  That said, that theory seems relatively unlikely.
Did you check that the various values set up in TimeStamp::Startup initialize to things that yield sensible results?
Has anyone tested on linux? That's the other place we use higher precision TimeStamping.
(In reply to Jeff Muizelaar [:jrmuizel] from comment #20)
> Has anyone tested on linux? That's the other place we use higher precision
> TimeStamping.

I can't reproduce the bug on Linux.
Neither can I.
> Did you check that the various values set up in TimeStamp::Startup initialize to things
> that yield sensible results?

With my code from comment 18, I get:

  NS PER TICK: 1.000000
  RESOLUTION: 1000000
  RESOLUTION SIG DIGS: 1000000

which I think is expected.
OK, so if I disable interruptible reflow (GECKO_REFLOW_INTERRUPT_MODE=counter GECKO_REFLOW_INTERRUPT_FREQUENCY=1000000 GECKO_REFLOW_INTERRUPT_CHECKS_TO_SKIP=1000000) then the issue goes away.

So presumably we're interrupting the reflow in this case for some reason with the new timestamp setup?
Aha.  nsPresContext.cpp has:

  static TimeDuration sInterruptTimeout = TimeDuration::FromMilliseconds(100);

This is, of course, happening before TimeStamp::Startup.  So sInterruptTimeout gets set to some random value.  Looking at mine in gdb right now, I see:

(gdb) p sInterruptTimeout 
$1 = {
  mValue = -9223372036854775808
}

Which means that this code in nsPresContext::CheckForInterrupt:

  mHasPendingInterrupt =
    TimeStamp::Now() - mReflowStartTime > sInterruptTimeout &&
    HavePendingInputEvent() &&
    !IsChrome();

sets mHasPendingInterrupt to true no matter what mReflowStartTime is.
Created attachment 566305 [details] [diff] [review]
Don't use TimeDuration methods from static constructors.
Attachment #566305 - Flags: review?(roc)
Assignee: nobody → bzbarsky
Priority: -- → P2
Whiteboard: [need review]
Hmm, so there's no way we can make sure that no other code is ever going to use this pattern.  Should we just switch the backend implementation to initialize lazily?  Or at least make it abort if something is called on it before the initialization happens?  If we don't do this, we're going to have to fight this issue again in the future...
Attachment #566305 - Flags: review?(roc) → review+
Yeah, it would be good to file a followup on making it impossible to shoot one's foot like this.
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff1d493bf113
Flags: in-testsuite?
Whiteboard: [need review]
Target Milestone: --- → mozilla10
I filed bug 693821.
https://hg.mozilla.org/mozilla-central/rev/ff1d493bf113
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Blocks: 693821

Updated

6 years ago
tracking-firefox8: ? → -
(Reporter)

Comment 32

5 years ago
Ah, I didn't realize this was landed for FF10.  Can someone point out to me how to workaround this until then?  Any kind of CSS trick I can employ to make the page flow smoothly?
Not a CSS one, but you could get a layout property (e.g. |document.body.offsetWidth|) right after your innerHTML set; that should work around the problem, at the cost of forcing some work to be synchronous that would normally get batched.
(Reporter)

Comment 34

5 years ago
Excellent!  Thanks, Boris - you're always so helpful :)
Comment on attachment 566305 [details] [diff] [review]
Don't use TimeDuration methods from static constructors.

We should strongly consider getting this in for Firefox 9.  I'm sorry it missed 8; I had been waiting for the tracking-* triage, but that never happened.  :(

The impact of this bug is that on Mac we always try to interrupt reflow as early as we can.  The result is almost certainly too many reflows, wasting CPU and taking longer to display pages.

The patch is very very safe: it just moves initialization of a variable from application startup to the first time the variable is used.
Attachment #566305 - Flags: approval-mozilla-beta?

Updated

5 years ago
status-firefox8: --- → affected
tracking-firefox10: ? → +
tracking-firefox9: ? → +

Comment 36

5 years ago
Comment on attachment 566305 [details] [diff] [review]
Don't use TimeDuration methods from static constructors.

[triage comment]
Approved for beta. Please land as soon as possible.
Attachment #566305 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 37

5 years ago
http://hg.mozilla.org/releases/mozilla-beta/rev/e7cd4b3f47d6

This looks to already be on aurora.
status-firefox10: --- → fixed
status-firefox9: --- → fixed
Yes, this was already on Aurora.  Thank you for pushing this to beta!
Whiteboard: [qa+]

Comment 39

5 years ago
Created attachment 579636 [details]
screenshot

Hi guys.

I have verified this bug on:
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:9.0) Gecko/20100101 Firefox/9.0 beta 4
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:10.0a2) Gecko/20111206 Firefox/10.0a2
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:11.0a1) Gecko/20111206 Firefox/11.0a1

The page doesn't jump anymore when I insert the text from the button, but I have to manually scroll the page to see the whole text.
When I clicked the button, the text appears as in the screenshot.

Is this intended? 
Thanks
I believe this is FIXED as described.

From comment 0:
"My site has an infinite-scroll feature that adds more content to the bottom of the page when you get there...there seems to be a regression that causes the page to jump/scroll to a point higher up in the document when this happens."

If it does not JUMP then this can be marked VERIFIED.
Status: RESOLVED → VERIFIED
Keywords: verified-aurora, verified-beta
Whiteboard: [qa+] → [qa!]
> Is this intended? 

Yes/
You need to log in before you can comment on or make changes to this bug.