Closed Bug 758361 Opened 12 years ago Closed 12 years ago

Viewport resets during page load

Categories

(Firefox for Android Graveyard :: General, defect)

Product:
Firefox for Android Graveyard
Component:
General
Platform:
All
Android
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox14 verified, firefox15 verified, firefox16 verified, blocking-fennec1.0 .N+)

Status:
VERIFIED FIXED
Milestone:
Firefox 16
Tracking Flags:
Tracking Status
firefox14 --- verified
firefox15 --- verified
firefox16 --- verified
blocking-fennec1.0 --- .N+

People

(Reporter: kats, Assigned: kats)

References

Details

(Whiteboard: [viewport])

Attachments

(2 files, 2 obsolete files)

Backtraces
12.82 KB, text/plain
Details
Move the before-first-paint into UnsuppressAndInvalidate
5.48 KB, patch
bzbarsky
: review+
mfinkle
: approval-mozilla-aurora+
blassey
: approval-mozilla-beta+
Details | Diff | Splinter Review 
STR:
1. Load CNN.com
2. While it's loading, zoom in and scroll down a bit

Expected results:
- When the page is done loading, it remains zoomed in

Actual results:
- It zooms out during page load

Seeing this on a Galaxy Nexus on m-c code, not sure if it's happening in aurora as well.
The reason this is happening is because the before-first-paint event is getting dispatched multiple times. The first time it is dispatched via the normal code path:

#0  DocumentViewerImpl::Show (this=<optimized out>) at /Users/kats/zspace/mozilla-git/layout/base/nsDocumentViewer.cpp:2031
#1  0x623846f2 in nsPresContext::EnsureVisible (this=0x616e3c00) at /Users/kats/zspace/mozilla-git/layout/base/nsPresContext.cpp:1811
#2  0x6238fc5a in PresShell::UnsuppressAndInvalidate (this=0x6183ed00) at /Users/kats/zspace/mozilla-git/layout/base/nsPresShell.cpp:3530
#3  0x62391a3e in PresShell::UnsuppressPainting (this=0x6183ed00) at /Users/kats/zspace/mozilla-git/layout/base/nsPresShell.cpp:3580
#4  0x6238ab1c in PresShell::sPaintSuppressionCallback (aTimer=<optimized out>, aPresShell=<optimized out>) at /Users/kats/zspace/mozilla-git/layout/base/nsPresShell.cpp:1709
#5  0x62da4882 in nsTimerImpl::Fire (this=0x658e1b00) at /Users/kats/zspace/mozilla-git/xpcom/threads/nsTimerImpl.cpp:473
#6  0x62da4a84 in nsTimerEvent::Run (this=0x5c6e2200) at /Users/kats/zspace/mozilla-git/xpcom/threads/nsTimerImpl.cpp:556
#7  0x62da0af2 in nsThread::ProcessNextEvent (this=0x5c6b10f0, mayWait=<optimized out>, result=0x5c4a87cf) at /Users/kats/zspace/mozilla-git/xpcom/threads/nsThread.cpp:624
#8  0x62d6a6e8 in NS_ProcessNextEvent_P (thread=0x5c60a0cc, mayWait=false) at /Users/kats/zspace/mozilla-git/obj-android-debug/xpcom/build/nsThreadUtils.cpp:213
#9  0x62cb2ad0 in mozilla::ipc::MessagePump::Run (this=0x5c6b3220, aDelegate=0x5c6d80e0) at /Users/kats/zspace/mozilla-git/ipc/glue/MessagePump.cpp:82
#10 0x62dd29a2 in MessageLoop::RunInternal (this=0x5c6d80e0) at /Users/kats/zspace/mozilla-git/ipc/chromium/src/base/message_loop.cc:208
#11 0x62dd2a02 in RunHandler (this=<optimized out>) at /Users/kats/zspace/mozilla-git/ipc/chromium/src/base/message_loop.cc:201
#12 MessageLoop::Run (this=0x5c6d80e0) at /Users/kats/zspace/mozilla-git/ipc/chromium/src/base/message_loop.cc:175
#13 0x62c04fae in nsBaseAppShell::Run (this=0x5f3303e0) at /Users/kats/zspace/mozilla-git/widget/xpwidgets/nsBaseAppShell.cpp:163
#14 0x62abfe7a in nsAppStartup::Run (this=0x5fd5a340) at /Users/kats/zspace/mozilla-git/toolkit/components/startup/nsAppStartup.cpp:256
#15 0x621ecef2 in XREMain::XRE_mainRun (this=0x5c4a8a74) at /Users/kats/zspace/mozilla-git/toolkit/xre/nsAppRunner.cpp:3765
#16 0x621ef5cc in XREMain::XRE_main (this=0x5c4a8a74, argc=<optimized out>, argv=0x5c6bf048, aAppData=0x5b6cda2c) at /Users/kats/zspace/mozilla-git/toolkit/xre/nsAppRunner.cpp:3842
#17 0x621ef756 in XRE_main (argc=7, argv=0x5c6bf048, aAppData=0x5b6cda2c) at /Users/kats/zspace/mozilla-git/toolkit/xre/nsAppRunner.cpp:3918
#18 0x621f443a in GeckoStart (data=0x1800308, appData=0x5b6cda2c) at /Users/kats/zspace/mozilla-git/toolkit/xre/nsAndroidStartup.cpp:73
#19 0x5b6b5aea in Java_org_mozilla_gecko_GeckoAppShell_nativeRun (jenv=0x1617d90, jc=<optimized out>, jargs=0x25f00005) at /Users/kats/zspace/mozilla-git/mozglue/android/APKOpen.cpp:966
#20 0x40843c34 in dvmPlatformInvoke () from /Users/kats/android/jdb/moz-gdb/lib/01466E640801401C/system/lib/libdvm.so
#21 0x4087deee in dvmCallJNIMethod(unsigned int const*, JValue*, Method const*, Thread*) () from /Users/kats/android/jdb/moz-gdb/lib/01466E640801401C/system/lib/libdvm.so


On subsequent dispatches the backtrace looks different, like this:

#0  DocumentViewerImpl::Show (this=<optimized out>) at /Users/kats/zspace/mozilla-git/layout/base/nsDocumentViewer.cpp:2031
#1  0x62a42a64 in nsDocShell::SetVisibility (this=<optimized out>, aVisibility=<optimized out>) at /Users/kats/zspace/mozilla-git/docshell/base/nsDocShell.cpp:5002
#2  0x6254d728 in nsFrameLoader::Show (this=0x6552f280, marginWidth=<optimized out>, marginHeight=<optimized out>, scrollbarPrefX=<optimized out>, scrollbarPrefY=2, frame=0x67eb9858)
    at /Users/kats/zspace/mozilla-git/content/base/src/nsFrameLoader.cpp:789
#3  0x62410e1a in nsSubDocumentFrame::ShowViewer (this=0x67eb9858) at /Users/kats/zspace/mozilla-git/layout/generic/nsSubDocumentFrame.cpp:197
#4  0x62410e64 in AsyncFrameInit::Run (this=<optimized out>) at /Users/kats/zspace/mozilla-git/layout/generic/nsSubDocumentFrame.cpp:113
#5  0x62514c1c in nsContentUtils::RemoveScriptBlocker () at /Users/kats/zspace/mozilla-git/content/base/src/nsContentUtils.cpp:4760
#6  0x62344f5e in nsAutoScriptBlocker::~nsAutoScriptBlocker (this=0x659952fc, __in_chrg=<optimized out>) at ../../dist/include/nsContentUtils.h:2163
#7  0x6239a5ae in PresShell::FlushPendingNotifications (this=0x6183ed00, aType=Flush_Style) at /Users/kats/zspace/mozilla-git/layout/base/nsPresShell.cpp:3800
#8  0x6239c72a in nsRefreshDriver::Notify (this=0x5fd04580, aTimer=<optimized out>) at /Users/kats/zspace/mozilla-git/layout/base/nsRefreshDriver.cpp:375
#9  0x62da488e in nsTimerImpl::Fire (this=0x658e1ab0) at /Users/kats/zspace/mozilla-git/xpcom/threads/nsTimerImpl.cpp:476
#10 0x62da4a84 in nsTimerEvent::Run (this=0x5c6e2460) at /Users/kats/zspace/mozilla-git/xpcom/threads/nsTimerImpl.cpp:556
#11 0x62da0af2 in nsThread::ProcessNextEvent (this=0x5c6b10f0, mayWait=<optimized out>, result=0x5c4a87cf) at /Users/kats/zspace/mozilla-git/xpcom/threads/nsThread.cpp:624
#12 0x62d6a6e8 in NS_ProcessNextEvent_P (thread=0x659952fc, mayWait=false) at /Users/kats/zspace/mozilla-git/obj-android-debug/xpcom/build/nsThreadUtils.cpp:213
#13 0x62cb2ad0 in mozilla::ipc::MessagePump::Run (this=0x5c6b3220, aDelegate=0x5c6d80e0) at /Users/kats/zspace/mozilla-git/ipc/glue/MessagePump.cpp:82
#14 0x62dd29a2 in MessageLoop::RunInternal (this=0x5c6d80e0) at /Users/kats/zspace/mozilla-git/ipc/chromium/src/base/message_loop.cc:208
#15 0x62dd2a02 in RunHandler (this=<optimized out>) at /Users/kats/zspace/mozilla-git/ipc/chromium/src/base/message_loop.cc:201
#16 MessageLoop::Run (this=0x5c6d80e0) at /Users/kats/zspace/mozilla-git/ipc/chromium/src/base/message_loop.cc:175
#17 0x62c04fae in nsBaseAppShell::Run (this=0x5f3303e0) at /Users/kats/zspace/mozilla-git/widget/xpwidgets/nsBaseAppShell.cpp:163
#18 0x62abfe7a in nsAppStartup::Run (this=0x5fd5a340) at /Users/kats/zspace/mozilla-git/toolkit/components/startup/nsAppStartup.cpp:256
#19 0x621ecef2 in XREMain::XRE_mainRun (this=0x5c4a8a74) at /Users/kats/zspace/mozilla-git/toolkit/xre/nsAppRunner.cpp:3765
#20 0x621ef5cc in XREMain::XRE_main (this=0x5c4a8a74, argc=<optimized out>, argv=0x5c6bf048, aAppData=0x5b6cda2c) at /Users/kats/zspace/mozilla-git/toolkit/xre/nsAppRunner.cpp:3842
#21 0x621ef756 in XRE_main (argc=7, argv=0x5c6bf048, aAppData=0x5b6cda2c) at /Users/kats/zspace/mozilla-git/toolkit/xre/nsAppRunner.cpp:3918
#22 0x621f443a in GeckoStart (data=0x1800308, appData=0x5b6cda2c) at /Users/kats/zspace/mozilla-git/toolkit/xre/nsAndroidStartup.cpp:73
#23 0x5b6b5aea in Java_org_mozilla_gecko_GeckoAppShell_nativeRun (jenv=0x1617d90, jc=<optimized out>, jargs=0x25f00005) at /Users/kats/zspace/mozilla-git/mozglue/android/APKOpen.cpp:966
#24 0x40843c34 in dvmPlatformInvoke () from /Users/kats/android/jdb/moz-gdb/lib/01466E640801401C/system/lib/libdvm.so
#25 0x4087deee in dvmCallJNIMethod(unsigned int const*, JValue*, Method const*, Thread*) () from /Users/kats/android/jdb/moz-gdb/lib/01466E640801401C/system/lib/libdvm.so
Sorry, the second backtrace above is wrong; that gets fired on a different document and is ignored. The correct backtrace looks like this:

#0  nsBeforeFirstPaintDispatcher::Run (this=0x672b1e00) at /Users/kats/zspace/mozilla-git/layout/base/nsDocumentViewer.cpp:4386
#1  0x6125198a in nsContentUtils::AddScriptRunner (aRunnable=0x672b1e00) at /Users/kats/zspace/mozilla-git/content/base/src/nsContentUtils.cpp:4781
#2  0x610af558 in DocumentViewerImpl::Show (this=<optimized out>) at /Users/kats/zspace/mozilla-git/layout/base/nsDocumentViewer.cpp:2039
#3  0x610bf742 in nsPresContext::EnsureVisible (this=0x661eec00) at /Users/kats/zspace/mozilla-git/layout/base/nsPresContext.cpp:1811
#4  0x618b217e in nsPluginInstanceOwner::Init (this=0x661f8900, aContent=0x65738c60) at /Users/kats/zspace/mozilla-git/dom/plugins/base/nsPluginInstanceOwner.cpp:3219
#5  0x618a6c7c in nsPluginHost::InstantiateEmbeddedPluginInstance (this=<optimized out>, aMimeType=0x6903c138 "application/x-shockwave-flash", aURL=0x65927b40, aContent=<optimized out>, 
    aOwner=0x65738d1c) at /Users/kats/zspace/mozilla-git/dom/plugins/base/nsPluginHost.cpp:905
#6  0x612aa234 in nsObjectLoadingContent::InstantiatePluginInstance (this=0x65738cb0, aMimeType=0x6903c138 "application/x-shockwave-flash", aURI=0x65927b40)
    at /Users/kats/zspace/mozilla-git/content/base/src/nsObjectLoadingContent.cpp:683
#7  0x612aa4ba in nsObjectLoadingContent::SyncStartPluginInstance (this=0x65738cb0) at /Users/kats/zspace/mozilla-git/content/base/src/nsObjectLoadingContent.cpp:2097
#8  0x6146a236 in nsHTMLPluginObjElementSH::GetPluginInstanceIfSafe (wrapper=<optimized out>, obj=<optimized out>, _result=0x5c3a3404) at /Users/kats/zspace/mozilla-git/dom/base/nsDOMClassInfo.cpp:9582
#9  0x6146f756 in nsHTMLPluginObjElementSH::NewResolve (this=0x65dab680, wrapper=0x66342c80, cx=0x5dd52da0, obj=0x66036b20, id=..., flags=5, objp=0x5c3a3510, _retval=0x5c3a3517)
    at /Users/kats/zspace/mozilla-git/dom/base/nsDOMClassInfo.cpp:9903
#10 0x6170de62 in XPC_WN_Helper_NewResolve (cx=0x5dd52da0, obj=..., id=..., flags=<optimized out>, objp=0x5c3a3594) at /Users/kats/zspace/mozilla-git/js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1069
#11 0x61e4f68c in CallResolveOp (recursedp=<optimized out>, propp=<optimized out>, objp=<optimized out>, flags=<optimized out>, id=<optimized out>, obj=<optimized out>, start=0x0, cx=<optimized out>)
    at /Users/kats/zspace/mozilla-git/js/src/jsobj.cpp:4625
#12 LookupPropertyWithFlagsInline (cx=0x5dd52da0, obj=..., id=..., flags=1628072405, objp=0x5c3a35e8, propp=0x5c3a361c) at /Users/kats/zspace/mozilla-git/js/src/jsobj.cpp:4675
#13 0x61e55e9e in js_GetPropertyHelperInline (vp=<optimized out>, getHow=<optimized out>, id_=<optimized out>, receiver=<optimized out>, obj=<optimized out>, cx=<optimized out>)
    at /Users/kats/zspace/mozilla-git/js/src/jsobj.cpp:5010
#14 js::GetPropertyHelper (cx=0x672b1e00, obj=..., id=<optimized out>, getHow=1, vp=0x5c3a3b28) at /Users/kats/zspace/mozilla-git/js/src/jsobj.cpp:5096
#15 0x61e2458a in GetPropertyOperation (vp=<optimized out>, lval=<optimized out>, pc=<optimized out>, cx=<optimized out>) at /Users/kats/zspace/mozilla-git/js/src/jsinterpinlines.h:231
#16 js::Interpret (cx=<optimized out>, entryFrame=<optimized out>, interpMode=<optimized out>) at /Users/kats/zspace/mozilla-git/js/src/jsinterp.cpp:2408
#17 0x61e2f33c in js::RunScript (cx=0x5dd52da0, script=<optimized out>, fp=0x5e100038) at /Users/kats/zspace/mozilla-git/js/src/jsinterp.cpp:266
#18 0x61e2fc4a in js::InvokeKernel (cx=0x5dd52da0, args=..., construct=js::NO_CONSTRUCT) at /Users/kats/zspace/mozilla-git/js/src/jsinterp.cpp:326
#19 0x61e30462 in Invoke (construct=<optimized out>, args=<optimized out>, cx=<optimized out>) at /Users/kats/zspace/mozilla-git/js/src/jsinterp.h:125
#20 js::Invoke (cx=0x5dd52da0, thisv=..., fval=..., argc=<optimized out>, argv=0x5c3a4108, rval=0x5c3a4268) at /Users/kats/zspace/mozilla-git/js/src/jsinterp.cpp:358
#21 0x61d811a0 in JS_CallFunctionValue (cx=0x5dd52da0, obj=0x671ee180, fval=..., argc=1, argv=0x5c3a4108, rval=0x5c3a4268) at /Users/kats/zspace/mozilla-git/js/src/jsapi.cpp:5491
#22 0x61702402 in nsXPCWrappedJSClass::CallMethod (this=0x5e609e20, wrapper=<optimized out>, methodIndex=<optimized out>, info=0x5de5f320, nativeParams=0x5c3a4350)
    at /Users/kats/zspace/mozilla-git/js/xpconnect/src/XPCWrappedJSClass.cpp:1474
#23 0x616fd2f2 in nsXPCWrappedJS::CallMethod (this=0x65441ec0, methodIndex=3, info=0x5de5f320, params=<optimized out>) at /Users/kats/zspace/mozilla-git/js/xpconnect/src/XPCWrappedJS.cpp:579
#24 0x61af0db6 in PrepareAndDispatch (self=<optimized out>, methodIndex=<optimized out>, args=0x5c3a440c) at /Users/kats/zspace/mozilla-git/xpcom/reflect/xptcall/src/md/unix/xptcstubs_arm.cpp:105
#25 0x61af03ec in SharedStub () from /Users/kats/zspace/mozilla-git/obj-android-debug/dist/bin/libxul.so
#26 0x6131e454 in nsEventListenerManager::HandleEventSubType (this=<optimized out>, aListenerStruct=<optimized out>, aListener=0x63cf06d0, aDOMEvent=0x65b9b4c0, aCurrentTarget=0x65a0cc00, 
    aPhaseFlags=6, aPusher=0x5c3a4594) at /Users/kats/zspace/mozilla-git/content/events/src/nsEventListenerManager.cpp:809
#27 0x6131e5de in nsEventListenerManager::HandleEventInternal (this=0x65899650, aPresContext=<optimized out>, aEvent=0x65b9b580, aDOMEvent=0x5c3a4584, aCurrentTarget=0x65a0cc00, aFlags=6, 
    aEventStatus=0x5c3a4588, aPusher=0x5c3a4594) at /Users/kats/zspace/mozilla-git/content/events/src/nsEventListenerManager.cpp:866
#28 0x613368c4 in HandleEvent (aPusher=<optimized out>, aEventStatus=<optimized out>, aFlags=<optimized out>, aCurrentTarget=<optimized out>, aDOMEvent=<optimized out>, aEvent=<optimized out>, 
    aPresContext=<optimized out>, this=<optimized out>) at /Users/kats/zspace/mozilla-git/content/events/src/nsEventListenerManager.h:137
#29 nsEventTargetChainItem::HandleEvent (this=<optimized out>, aVisitor=..., aFlags=6, aMayHaveNewListenerManagers=<optimized out>, aPusher=0x5c3a4594)
    at /Users/kats/zspace/mozilla-git/content/events/src/nsEventDispatcher.cpp:185
#30 0x61336a30 in nsEventTargetChainItem::HandleEventTargetChain (this=<optimized out>, aVisitor=..., aFlags=6, aCallback=0x0, aMayHaveNewListenerManagers=false, aPusher=0x5c3a4594)
    at /Users/kats/zspace/mozilla-git/content/events/src/nsEventDispatcher.cpp:317
#31 0x613373f8 in nsEventDispatcher::Dispatch (aTarget=<optimized out>, aPresContext=0x661eec00, aEvent=0x65b9b580, aDOMEvent=<optimized out>, aEventStatus=0x5c3a4630, aCallback=0x0, aTargets=0x0)
    at /Users/kats/zspace/mozilla-git/content/events/src/nsEventDispatcher.cpp:643
#32 0x613376f4 in nsEventDispatcher::DispatchDOMEvent (aTarget=0x65a0cc00, aEvent=<optimized out>, aDOMEvent=0x65b9b4c0, aPresContext=0x661eec00, aEventStatus=0x5c3a4630)
    at /Users/kats/zspace/mozilla-git/content/events/src/nsEventDispatcher.cpp:706
#33 0x6129897a in nsINode::DispatchEvent (this=0x65a0cc00, aEvent=0x65b9b4c0, aRetVal=0x5c3a4687) at /Users/kats/zspace/mozilla-git/content/base/src/nsGenericElement.cpp:1132
#34 0x61246d9c in nsContentUtils::DispatchEvent (aDoc=<optimized out>, aTarget=<optimized out>, aEventName=<optimized out>, aCanBubble=<optimized out>, aCancelable=true, aTrusted=true, 
    aDefaultAction=0x5c3a4687) at /Users/kats/zspace/mozilla-git/content/base/src/nsContentUtils.cpp:3397
#35 0x61246dfe in nsContentUtils::DispatchTrustedEvent (aDoc=0x672b1e00, aTarget=0x65b9b4c0, aEventName=..., aCanBubble=241, aCancelable=<optimized out>, aDefaultAction=0x0)
    at /Users/kats/zspace/mozilla-git/content/base/src/nsContentUtils.cpp:3368
#36 0x6127e62c in nsDocument::DispatchContentLoadedEvents (this=0x65a0cc00) at /Users/kats/zspace/mozilla-git/content/base/src/nsDocument.cpp:4148
#37 0x60f4b3ae in nsRunnableMethodImpl<void (nsPACMan::*)(), true>::Run (this=<optimized out>) at ../../../dist/include/nsThreadUtils.h:313
#38 0x61adbb62 in nsThread::ProcessNextEvent (this=0x5c5b10f0, mayWait=<optimized out>, result=0x5c3a47cf) at /Users/kats/zspace/mozilla-git/xpcom/threads/nsThread.cpp:624
#39 0x61aa5758 in NS_ProcessNextEvent_P (thread=0x672b1e00, mayWait=false) at /Users/kats/zspace/mozilla-git/obj-android-debug/xpcom/build/nsThreadUtils.cpp:213
#40 0x619edb40 in mozilla::ipc::MessagePump::Run (this=0x5c5b3220, aDelegate=0x5c5d80e0) at /Users/kats/zspace/mozilla-git/ipc/glue/MessagePump.cpp:82
#41 0x61b0da1a in MessageLoop::RunInternal (this=0x5c5d80e0) at /Users/kats/zspace/mozilla-git/ipc/chromium/src/base/message_loop.cc:208
#42 0x61b0da7a in RunHandler (this=<optimized out>) at /Users/kats/zspace/mozilla-git/ipc/chromium/src/base/message_loop.cc:201
#43 MessageLoop::Run (this=0x5c5d80e0) at /Users/kats/zspace/mozilla-git/ipc/chromium/src/base/message_loop.cc:175
#44 0x6194001e in nsBaseAppShell::Run (this=0x5dd303e0) at /Users/kats/zspace/mozilla-git/widget/xpwidgets/nsBaseAppShell.cpp:163
#45 0x617faeea in nsAppStartup::Run (this=0x5e65a340) at /Users/kats/zspace/mozilla-git/toolkit/components/startup/nsAppStartup.cpp:256
#46 0x60f27ef2 in XREMain::XRE_mainRun (this=0x5c3a4a74) at /Users/kats/zspace/mozilla-git/toolkit/xre/nsAppRunner.cpp:3765
#47 0x60f2a5cc in XREMain::XRE_main (this=0x5c3a4a74, argc=<optimized out>, argv=0x5c5bf048, aAppData=0x5b4cda2c) at /Users/kats/zspace/mozilla-git/toolkit/xre/nsAppRunner.cpp:3842
#48 0x60f2a756 in XRE_main (argc=7, argv=0x5c5bf048, aAppData=0x5b4cda2c) at /Users/kats/zspace/mozilla-git/toolkit/xre/nsAppRunner.cpp:3918
#49 0x60f2f43a in GeckoStart (data=0x19750c0, appData=0x5b4cda2c) at /Users/kats/zspace/mozilla-git/toolkit/xre/nsAndroidStartup.cpp:73
#50 0x5b4b5aea in Java_org_mozilla_gecko_GeckoAppShell_nativeRun (jenv=0x1762020, jc=<optimized out>, jargs=0x25f00005) at /Users/kats/zspace/mozilla-git/mozglue/android/APKOpen.cpp:966
#51 0x40843c34 in dvmPlatformInvoke () from /Users/kats/android/jdb/moz-gdb/lib/01466E640801401C/system/lib/libdvm.so
#52 0x4087deee in dvmCallJNIMethod(unsigned int const*, JValue*, Method const*, Thread*) () from /Users/kats/android/jdb/moz-gdb/lib/01466E640801401C/system/lib/libdvm.so
#53 0x01762128 in ?? ()
#54 0x01762128 in ?? ()
blocking-fennec1.0: --- → ?
Whiteboard: [viewport]
snorp, can you shed any light on the backtrace in comment 2? Specifically if anything in that path changed recently (last ~2 weeks or so) that might be causing DocumentViewerImpl::Show to get called when it didn't before? I just want to understand the problem better. It's likely that sticking in a guard somewhere to not show the document twice will fix this problem, but I want to make sure there's no other underlying problems.
I'm not aware of any changes to that code, but I don't really keep tabs on the general plugin stuff. I think maybe there were some click-to-play changes, but not sure if they touched this path.
This bug really irritates me.

Looking at it briefly it does indeed seem like there should be a guard in nsDocumentViewer::Show. I don't see anything obvious to use for that, though...
Attached file Backtraces — 
Putting the backtraces in an attachment so they're easier to read.
This is smallest guard that gets the job done. :bz, I'm open to suggestions for better fixes for this - I just don't know this code very well and figured it would be easier to discuss once I verified this fixes the problem (which it does).
Attachment #627270 - Flags: review?(bzbarsky)
What's the expected behavior if a viewer is shown, then hidden, then shown again?
The browser.js code that uses the event expects the event to come only once over the entire lifetime of a document. So if the viewer is shown, then hidden, then shown again, I don't think the second shown should trigger the event.

However I don't know all the circumstances under which the viewer might be hidden. Is there an easy way to know what those are?
Sure.  display:none on the containing frame (or anything that triggers a frame reconstruct on it) will do it.
In those cases we don't want the before-first-paint event to fire.
Comment on attachment 627270 [details] [diff] [review]
Add a guard for before-first-paint

OK.  What about the case when the hide is due to going into bfcache and the show is due to coming out of bfcache?
Hmm.. yeah in that case we want the event.
blocking-fennec1.0: ? → soft
Soft, but yesplz
Comment on attachment 627270 [details] [diff] [review]
Add a guard for before-first-paint

Then this doesn't sound like what you want....
Attachment #627270 - Flags: review?(bzbarsky) → review+
Is there some way to detect that case? Or a better place/API to hook for this purpose? There must be some point during page load at which the active document is switched from the old one to the new one (where "active" is defined as "the document that will be painted if a paint is requested").
> Is there some way to detect that case?

Probably, with enough work.  Most simply, by changing the signature of Show() and fixing all callers as needed.

> There must be some point during page load at which the active document is switched from
> the old one to the new one (where "active" is defined as "the document that will be
> painted if a paint is requested").

There is, and that point is Show().  It's just that this point is also reached in various other cases (e.g. when a document switches from display:none to not).  Preexisting consumers don't care about the differences between those two cases; they just care that the document used to not need to be painted, and now it needs to be paintes.
Modifying the Show() function to take a parameter requires IDL changes and will have far-reaching implications, so I'd like to avoid that. The attached patch moves the dispatching of the before-first-paint event up a level in the call stack, so that it only happens on the code path with the UnsuppressAndInvalidate call, rather than also happening on the plugin codepath.

Based on your previous comments I suspect this will get triggered on frame reconstruction as well, but I realized that in practice this doesn't matter because (1) our browser.js listener only cares about these events for the top-level content document, not iframes and (2) I don't believe there is any way to really trigger a frame reconstruction on the top-level content document in Fennec.

Does that seem reasonable?
Attachment #627270 - Attachment is obsolete: true
Attachment #628003 - Flags: review?(bzbarsky)
bz, review ping?
Working on it.  I've been in meetings most of the last two days.  :(  Should hopefully happen tonight after the kids are in bed.
Comment on attachment 628003 [details] [diff] [review]
Move the before-first-paint into EnsureVisible

This will also get triggered by fullscreen, possibly.

In any case, why are you passing false in the nsPluginInstanceOwner code?
(In reply to Boris Zbarsky (:bz) from comment #21)
> This will also get triggered by fullscreen, possibly.

It doesn't appear to get triggered by fullscreening/unfullscreening. At least it didn't hit the breakpoint I set in EnsureVisible.

> In any case, why are you passing false in the nsPluginInstanceOwner code?

So that we don't fire the before-first-paint event on that codepath. As I understand it, that codepath may only execute after the document is already active.
> It doesn't appear to get triggered by fullscreening/unfullscreening.

Just on Fennec, right?  I would expect it to trigger for fullscreen in b2g or a XUL UI.

> As I understand it, that codepath may only execute after the document is already active.

If that were the case, the call to EnsureVisible() would not be needed at all, I would think.  The fact that it's needed exactly indicates that the document is not "already active", unless I'm really missing something.
(In reply to Boris Zbarsky (:bz) from comment #23)
> Just on Fennec, right?  I would expect it to trigger for fullscreen in b2g
> or a XUL UI.

I only tested that on Fennec, so yes.

> If that were the case, the call to EnsureVisible() would not be needed at
> all, I would think.  The fact that it's needed exactly indicates that the
> document is not "already active", unless I'm really missing something.

Based on the comment right above the call to EnsureVisible() it sounds like a workaround to make sure plugins get destroyed properly. A workaround that, as a side-effect, could end up calling EnsureVisible on a document that is already active. That's the entire problem in this bug, in fact - while loading CNN I see EnsureVisible get called two or three times for the top-level document, which is what is triggering the viewport reset.
What the comment says is that we can reach that code for a plug-in when the document of that plug-in is loading but has not yet been made active.  _If_ that happens, we have to make sure we make the previous document inactive (and destroy any plug-ins in it) before we proceed with plug-in setup for the new document.  To make the previous document inactive, we call EnsureVisible() on the document of the plug-in, on the assumption that it's a no-op if the document is already visible and will hide the old document if the new one is not visible yet.
(In reply to Boris Zbarsky (:bz) from comment #25)
> To make the previous document inactive, we call
> EnsureVisible() on the document of the plug-in, on the assumption that it's
> a no-op if the document is already visible

Right, exactly. The problem is that it's not a no-op for us, because it ends up firing the before-first-paint event again.

> and will hide the old document if
> the new one is not visible yet.

In this case there will still be another call to EnsureVisible from the usual code path (i.e. from UnsuppressAndInvalidate -- see the first backtrace in comment #1). As long as exactly one of these two calls to EnsureVisible triggers the before-first-paint event, it does what we want. Passing in false from the plug-in code and true from the other code makes that happen.
Hmm, ok.  But then why not just put the event dispatch in UnsuppressAndInvalidate, if that's what you really want?
I wanted to check the return value from Show() to make sure the event isn't dispatched if Show() fails. Although if you're ok with me changing EnsureVisible() to return false when Show() fails then I can move the dispatch to UnsuppressAndInvalidate.
I think I would be fine with that, yes.  That sounds like the best approach here.  Thanks for talking this through!
Thank you as well!

This moves the call out into PresShell::UnsuppressAndInvalidate and runs it if and only if the call to EnsureVisible happens and returns true.
Attachment #628003 - Attachment is obsolete: true
Attachment #628003 - Flags: review?(bzbarsky)
Attachment #629759 - Flags: review?(bzbarsky)
Comment on attachment 629759 [details] [diff] [review]
Move the before-first-paint into UnsuppressAndInvalidate

r=me
Attachment #629759 - Flags: review?(bzbarsky) → review+
This didn't make the merge today.
status-firefox15: fixed → affected
Target Milestone: Firefox 15 → Firefox 16
https://hg.mozilla.org/mozilla-central/rev/43c1e66ef550
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment on attachment 629759 [details] [diff] [review]
Move the before-first-paint into UnsuppressAndInvalidate

[Approval Request Comment]
Bug caused by (feature/regressing bug #): unknown
User impact if declined: on some pages with plugins, during page load the viewport jumps back up to the top of the page and zooms out
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): should affect mobile only, because mobile is the only code that listens for this event. however the code change itself affects all platforms. there is a small risk of it not fixing the bug completely
String or UUID changes made by this patch: none
Attachment #629759 - Flags: approval-mozilla-beta?
Attachment #629759 - Flags: approval-mozilla-aurora?
Attachment #629759 - Flags: approval-mozilla-beta?
Attachment #629759 - Flags: approval-mozilla-beta-
Attachment #629759 - Flags: approval-mozilla-aurora?
Attachment #629759 - Flags: approval-mozilla-aurora+
note: we're not OK with this landing in beta due to possible risk, but Aurora is OK by us and akeybl.
Comment on attachment 629759 [details] [diff] [review]
Move the before-first-paint into UnsuppressAndInvalidate

[Triage Comment]
Mobile triage decided that because this was only a soft blocker and carries non-zero risk, we'd approve for Aurora 15 but not Beta 14.
Depends on: 763166
No longer depends on: 763166
re-nom'ing and suggesting .N+ because it fixed the brightcove CDN problem
blocking-fennec1.0: soft → ?
blocking-fennec1.0: ? → .N+
Comment on attachment 629759 [details] [diff] [review]
Move the before-first-paint into UnsuppressAndInvalidate

Let's get this landed on mozilla-beta's default branch
Attachment #629759 - Flags: approval-mozilla-beta- → approval-mozilla-beta+
I am not seeing any viewport jump on Nightly 16.0a1 2012-06-19 or Aurora 15.0a2 2012-06-19 on HTC Desire running Android 2.2. The patch has not landed in Firefox Mobile 14 Beta 7 so leaving the bug opened for verification for Beta 8.
status-firefox15: fixed → verified
status-firefox16: fixed → verified
Verified/Fixed on:
Firefox Mobile 14.0b8 2012-06-22
HTC Desire Z - Android (2.3.3)
Status: RESOLVED → VERIFIED
status-firefox14: fixed → verified
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: