After some discussion with BenWa and botond we came to the conclusion that in order to best solve our displayport woes in bug 947523 for 1.3+ we should change the displayport representation in layout. Currently the displayport is set as a CSS-pixel rect that is relative to the scroll offset. Instead, we want it to be specified in layer pixels, and to be a set of margins that should be applied to the visible region of the frame. The assumption we have here is one that tn and I discussed on IRC a few days ago. The assumption is that the user's maximum panning speed is defined in layer pixels / second, which means the displayport margin should be roughly constant in layer pixels irrespective of what zoom level is being used. We considered a couple of other approaches, such as doing the displayport recomputation more frequently in the APZC code, but that leads to the possibility of race conditions. Storing the displayport as a percentage margin is also not as good because content scripts could arbitrarily change how much of the iframe is visible and result in unexpected changes to the displayport region. The one downside of storing the displayport as layerpixel margin values is that as more of the iframe becomes visible the displayport will change shape. Enabling tiling should mitigate this problem.

NOMing this for 1.3? since it addresses the b2g OOM originally identified in bug 947523. The fix in bug 947523 only addressed v1.2, so we need this to solve the issue for v1.3+.

Milan can you have someone on your team look into this?

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #0) > Instead, we want it to be specified in layer pixels, and to > be a set of margins that should be applied to the visible region of the > frame. We'll need to define exactly what we mean by "visible region" at some point.

(In reply to Timothy Nikkel (:tn) from comment #3) > We'll need to define exactly what we mean by "visible region" at some point. Yeah. In the context of this description I was referring to the region visible to the user. That is, it takes into account the clips of parent scroll frames. BenWa was saying that layout already computes this because without APZC/displayports that's what it ends up painting.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #0) > which means the displayport margin should be roughly > constant in layer pixels irrespective of what zoom level is being used. and constant in memory usage. > > Storing the displayport as a percentage > margin is also not as good because content scripts could arbitrarily change > how much of the iframe is visible and result in unexpected changes to the > displayport region. i.e. make the code fundamentally race-y. (In reply to Timothy Nikkel (:tn) from comment #3) > We'll need to define exactly what we mean by "visible region" at some point. And change layers' visible region to retain region which is really what they are :D

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #4) > (In reply to Timothy Nikkel (:tn) from comment #3) > > We'll need to define exactly what we mean by "visible region" at some point. > > Yeah. In the context of this description I was referring to the region > visible to the user. That is, it takes into account the clips of parent > scroll frames. BenWa was saying that layout already computes this because > without APZC/displayports that's what it ends up painting. Layout computes it if displayports aren't set, but if they are then we won't take into account the root scroll frame's clip for example, so anything in the display port will be included in this definition of visible.

Jet, can you send us somebody to help with this?

Jet asked me to add my thought on this, so here they are: (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #4) > (In reply to Timothy Nikkel (:tn) from comment #3) > > We'll need to define exactly what we mean by "visible region" at some point. > > Yeah. In the context of this description I was referring to the region > visible to the user. That is, it takes into account the clips of parent > scroll frames. BenWa was saying that layout already computes this because > without APZC/displayports that's what it ends up painting. What layout computes is actually a bit more than that. For example, when an opaque element covers parts of the scrollable subframe, that part will also be removed from the visible region. So the visible region can have a more complex shape than just a single rect (which is what you'd get if you only did the ancestor scrollframe clipping). (In reply to Timothy Nikkel (:tn) from comment #6) > Layout computes it if displayports aren't set, but if they are then we won't > take into account the root scroll frame's clip for example, so anything in > the display port will be included in this definition of visible. So then this is the code we'd change, right? Instead of overriding the visible region with a display port, we'd take the computed visible region, maybe simplify it to its bounding rect, then add the layerpixel margins, and propagate that result to the scrolled contents. And in case we have tiling, we'd also do the expand-to-tiles calculation here. At the moment that happens just before APZCCallbackHelper sets the display port. Two more questions come to mind: 1. What if the visible region of a scrollable subframe is empty, for example because the it is scrolled out of view or completely covered? I think we should keep the display port empty in that case, too. Then we won't waste any memory on invisible scroll frames, which is definitely desirable. 2. When determining whether nested scrollable frames are visible, should we consult its actual visible region or the one that has already been subjected to the margin expansion by the outer scroll frame? I'd argue for the latter: If the outer frame's prerender margins include parts of a scrollable subframe, that should get its contents prerendered, too, so that if you pan to it asynchronously you can start scrolling it right away without waiting for the main thread to catch up. It also maps to the code in a much more natural way.

(In reply to Markus Stange [:mstange] from comment #8) > What layout computes is actually a bit more than that. For example, when an > opaque element covers parts of the scrollable subframe, that part will also > be removed from the visible region. So the visible region can have a more > complex shape than just a single rect (which is what you'd get if you only > did the ancestor scrollframe clipping). Ah, I didn't know that. Thanks for the info! Everything else you said makes perfect sense to me as well. I might bug you a bit more later for pointers to code as to where these changes need to be made.

I'm going to start looking at this bug although I might have to hand it off to somebody in layout later.

This is a temporary fix which makes the browser and other things usable for me again on Hamachi. This should not be pushed to the tree unless all else fails, and I won't be pushing it following r+. kats has better plans for dealing with this which we should go with instead.

Comment on attachment 8361851 [details] [diff] [review] Shrink APZC margins to reduce OOMing on B2G Review of attachment 8361851 [details] [diff] [review]: ----------------------------------------------------------------- I'd rather change this in the b2g prefs file if it comes to that. That way metro won't be affected.

Talked to kats about this today some. For the rect to apply the margins to I think we want to use the intersection of the passed in clip to ScrollFrameHelper::BuildDisplayList, with the dirty rect, and the scrollport rect. If we were to use the visible rect of the display item or the visible region of the layer that would be more complex (the visible rect/region depend on the geometry of display items, but determining the displayport determines the geometry of some of the display items, so it is sort of a cycle dependency, it could probably be resolved, but I'm not sure the added complexity is worth it). I don't think opaque items covering part of a scroll frame affecting the displayport is going to be an important case. If it turns out to be we can always go with the more complicated solution later. We should store this rect somewhere so all users of displayport can access it. If we only need it during display list processing/construction we can store it on the display item. If we need it outside of those times we would probably have to store it as a property. Since display lists are build for more than just painting to the screen (drawwindow, hit testing) we'd want to make sure the stored rect from one type doesn't affect the painting to the screen display list.

(In reply to Timothy Nikkel (:tn) from comment #13) > For the rect to apply the margins to I > think we want to use the intersection of the passed in clip to > ScrollFrameHelper::BuildDisplayList, with the dirty rect, and the scrollport > rect. And this might be different from root scroll frames of root content documents. In those we might want to use the scroll position clamping scroll port size (we should probably rename this, it's being use for more than just that one purpose now) instead of the scrollport.

Looks like IsRectNearlyVisible (via UpdateImageVisibilityForFrame) need the display port, and that needs to work completely outside of anything display list based, so we'll need to store the property outside of the display item. On the frame, or perhaps on the content (alongside where we will store the displayport margins?).

IsRectNearlyVisible needs to work on pages that haven't painted (background tabs), so we will either need to give it a way to compute the displayport from the displayport margins, or base it off something it can compute easily.

For IsRectNearlyVisible, can we not just tack on the margins to the mScrollPort there?

There are some problems in this patch but it's what I started with

I have no idea what the proper changes for these bits will look like.

Comment on attachment 8364675 [details] [diff] [review] Part 5 (WIP) - Updates to layout I'm sure there's stuff in here that's obviously wrong to you. Please let me know what it is.

Comment on attachment 8364675 [details] [diff] [review] Part 5 (WIP) - Updates to layout So the resulting displayport computing in the nsDisplayList.cpp changes is based off mVisibleRect, which is the result of ComputeVisibility, ie opaquely covered areas are removed from it. So we will get a different result from when we compute the displayport in for example nsGfxScrollFrame.cpp. So I think we will need to store the base rect that we use to expand to create the display port. That works for scroll layer items. The PaintForFrame case is a little different. If we could use the dirty rect from it's caller, nsLayoutUtils::PaintFrame, that might work. Otherwise looks good.

What is the user impact of not fixing this in 1.3? We're hitting our deadlines, so just trying to ascertain if this is vital to fix.

Probably the main user-visible impact is bug 947523. That was fixed for 1.2 but this bug should fix it properly for 1.3 and up. There are also other less user-visible impacts, such as more efficient use of memory and possibly less checkerboarding with APZ panning.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #27) > Probably the main user-visible impact is bug 947523. That was fixed for 1.2 > but this bug should fix it properly for 1.3 and up. There are also other > less user-visible impacts, such as more efficient use of memory and possibly > less checkerboarding with APZ panning. I'm curious to see where we stand actually with 1.3 with that bug to see if the impact is still the same. QA Wanted - Can someone retest the STR on bug 947523 & indicate what happens here on 1.3?

I restart OS crashed after reproducing the STRs on bug 947523 on the latest nightly b2g28 branch of v1.3 which is identical to the actual results of that issue. v1.3 Environmental Variables: Device: Buri v1.3 MOZ BuildID: 20140203181708 Gaia: bb36b4164d3e51ca04b612e507e1178f80bf240d Gecko: 9731b0b7fa78 Version: 28.0 Firmware Version: v1.2-device.cfg

Can we get ETA to fix this bug? Thank you.

The mochitest in here let me test this code on desktop much more easily than I could test it on a device, so I'm including it here.

Handing this off to :tn for now since I've been too busy with other APZ things to make much progress here. Plus it's really slow going for me because I get don't understand this code as much, so hopefully tn can make more progress here. The attached patches are my latest WIPs. Part 5 I'm not really confident is right but it was the best I could come up with; part 6 is just a hack because I don't know what the right rect to be using at the call sites there is so I just commented it out. Part 7 I think is mostly correct but might need some debugging. I was using the mochitest + printfs in part 8 to verify that the displayport was getting propagated properly and expanded correctly in various places. If bug 961289 lands then the stuff in part 8 could actually be turned into a proper test and landed.

The size of the work involved here looks really risky for this point in the release, so I think we need to re-evaluate the blocking decision here. We know that we could consider porting bug 947523 to 1.3 to resolve bug 965945, so that could resolve that blocker. bug 942750 is also more of a meta bug, so unblocking this allows that bug to be finished after QA verification of the current state of checkboarding.

An offline discussion with Milan concluded that this is too risky to take to 1.3 at this point, so I'm punting this to 1.4 triage.

If APZC is being turned on in 1.3 but we don't have this, we likely want to add a temporary max displayport size, based on screen resolution perhaps. Otherwise we risk having apps crash after a user zooms in (when zooming is enabled, of course).

(In reply to Chris Lord [:cwiiis] from comment #42) > If APZC is being turned on in 1.3 but we don't have this, we likely want to > add a temporary max displayport size, based on screen resolution perhaps. > Otherwise we risk having apps crash after a user zooms in (when zooming is > enabled, of course). I think Kats already did this in bug 965945, and then did it better (no crashes) in bug 975033.

(In reply to Botond Ballo [:botond] from comment #43) > (In reply to Chris Lord [:cwiiis] from comment #42) > > If APZC is being turned on in 1.3 but we don't have this, we likely want to > > add a temporary max displayport size, based on screen resolution perhaps. > > Otherwise we risk having apps crash after a user zooms in (when zooming is > > enabled, of course). > > I think Kats already did this in bug 965945, and then did it better (no > crashes) in bug 975033. hmm, not quite... That disallows displayports bigger than 4096x4096, but a displayport of that size could require as much as 128mb of video memory if double-buffered, which is likely more than is going to be available on any device we're shipping on for a good while yet (the Peak, for example, has 128mb of ram total available for video memory). I think it'd be better to have a lower limit and rather than just remove the displayport in that case, make sure it stays within those bounds. At least for the tiles case anyway.

For bug posterity, Chris filed bug 979084 for the issue in comments 42-44.

Just using layer margins won't solve this bug. I outlined why in bug 982596, comment 0, which is the bug I filed with patches to do the other piece of work we'll need to not OOM.

Summary of this bug: there is a problem when a scrollable element other than the root has a display port, if the user pinch zooms the page the resolution will be increased. Since displayports are set in css pixels this means the displayport size for the scrollable elements increase by the same factor, and this can easily lead to displayports that are very large in actual pixel size. Leading to OOM. There is another related problem with the way APZC controller calculates displayports. It does so as a multiplier based on the composition bounds. For scrollable elements that is the amount of content that it will allow visible on the screen at any one time. However scrollable elements can be arbitrarily tall, so this calculation just isn't reasonable, and needs to be changed. We have some hacks that hardcode max sizes and a few other things to try to prevent these OOMs from occuring but they don't fix all the cases. kats is the author of these and would know how prevalent these problems still are with these hacks in place.

(In reply to Timothy Nikkel (:tn) from comment #47) > There is another related problem with the way APZC controller calculates > displayports. It does so as a multiplier based on the composition bounds. > For scrollable elements that is the amount of content that it will allow > visible on the screen at any one time. However scrollable elements can be > arbitrarily tall, so this calculation just isn't reasonable, and needs to be > changed. This part isn't true - the composition bounds are the amount of content that is visible on the screen, and are bound by the widget size, which is eventually bound by the screen size. The values we've chosen as multipliers work ok based on the screen/memory sizes of the devices we have at the moment.

(In reply to Chris Lord [:cwiiis] from comment #48) > (In reply to Timothy Nikkel (:tn) from comment #47) > > There is another related problem with the way APZC controller calculates > > displayports. It does so as a multiplier based on the composition bounds. > > For scrollable elements that is the amount of content that it will allow > > visible on the screen at any one time. However scrollable elements can be > > arbitrarily tall, so this calculation just isn't reasonable, and needs to be > > changed. > > This part isn't true - the composition bounds are the amount of content that > is visible on the screen, and are bound by the widget size, which is > eventually bound by the screen size. The values we've chosen as multipliers > work ok based on the screen/memory sizes of the devices we have at the > moment. For the composition bounds for the root scroll frame what you say is true, but for composition bounds for any other element they are just the size of the frame, which is controlled purely via css, so it can be arbitrarily big. This is why the problem only applies to other scrollable elements, and not the root.

(In reply to Timothy Nikkel (:tn) from comment #49) > (In reply to Chris Lord [:cwiiis] from comment #48) > > (In reply to Timothy Nikkel (:tn) from comment #47) > > > There is another related problem with the way APZC controller calculates > > > displayports. It does so as a multiplier based on the composition bounds. > > > For scrollable elements that is the amount of content that it will allow > > > visible on the screen at any one time. However scrollable elements can be > > > arbitrarily tall, so this calculation just isn't reasonable, and needs to be > > > changed. > > > > This part isn't true - the composition bounds are the amount of content that > > is visible on the screen, and are bound by the widget size, which is > > eventually bound by the screen size. The values we've chosen as multipliers > > work ok based on the screen/memory sizes of the devices we have at the > > moment. > > For the composition bounds for the root scroll frame what you say is true, > but for composition bounds for any other element they are just the size of > the frame, which is controlled purely via css, so it can be arbitrarily big. > This is why the problem only applies to other scrollable elements, and not > the root. Oh, interesting - what I said is, as I understand it, how it was intended to be. I guess that would still be problematic when you involve transforms, but that's certainly a problem then.

(In reply to Chris Lord [:cwiiis] from comment #50) > Oh, interesting - what I said is, as I understand it, how it was intended to > be. I guess that would still be problematic when you involve transforms, but > that's certainly a problem then. You don't even need transforms: <div style="height: 100000px; overflow: scroll;"> is enough.

(In reply to Timothy Nikkel (:tn) from comment #51) > (In reply to Chris Lord [:cwiiis] from comment #50) > > Oh, interesting - what I said is, as I understand it, how it was intended to > > be. I guess that would still be problematic when you involve transforms, but > > that's certainly a problem then. > > You don't even need transforms: <div style="height: 100000px; overflow: > scroll;"> is enough. I meant if it worked as it was intended (recursing and clipping on parents' composition bounds), you could still trigger this with transforms.

(In reply to Chris Lord [:cwiiis] from comment #52) > I meant if it worked as it was intended (recursing and clipping on parents' > composition bounds), you could still trigger this with transforms. This is easily fixed by taking the min of the composition bounds and the client size though.

Anyways, what you describe as the intended approach is what I basically did in bug 982596, by leaving the composition bounds alone and adding a new field. However that approach got rebuffed in that bug.

Current plan: 1) change how APZC calculates displayport. cap the maximize size of composition bounds we use to the composition bounds of the root APZC. This is the maximum amount of content that we can scroll into view quickly. But since the display port calculation has been tuned for root scroll frames (it scales based on the screen size only essentially) this just basically decides on layer margins using only the current velocity and screen size using the already tuned code extended logically to non-root scrollables. If we want to throw out our display port calculation code and start anew we can do that as well, and can easily be done separately once we have something that works reasonable. 2) Convert representation of displayports to layer margins + base rect. I think the api needs to be able to provide a display port rect to consumers, it doesn't make sense for every consumer to try to pull a base rect out of somewhere to apply margins to, it's too inconvenient. So APZC can set an initial base rect to the composition bounds (or something) just so there is some base rect, and then layout will update it properly every time it paints (the "visible" rect). So I don't have to update every display port user I'll add this margin api in parallel with the rect one, making the getting prefer the stored margin value if both are set. We can remove the old api when all uses are converted. You may all now proceed to tear this proposal apart. :)

Your proposal sounds very reasonable to me, fwiw.

Filed bug 986413 for part 1) of comment 55.

This also contains a bunch of odds and ends that should be in other patches.

Comment on attachment 8396316 [details] [diff] [review] the layer margins api Review of attachment 8396316 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsDOMWindowUtils.cpp @@ +312,5 @@ > static_cast<DisplayPortPropertyData*>(aPropertyValue); > delete data; > } > > +static void DestroyDisplayPortMarginsPropertyData(void* aObject, nsIAtom* aPropertyName, Bug 983285 just landed on inbound this morning when you pick that up you can replace this function with using the template function. ::: layout/base/nsLayoutUtils.cpp @@ +684,5 @@ > + } > + > + nsIFrame* frame = aContent->GetPrimaryFrame(); > + if (frame) { > + // first conver the base rect to layer pixels s/conver/convert/ @@ +706,5 @@ > + marginsData->mAlignment * floor(rect.y / marginsData->mAlignment); > + float right = > + marginsData->mAlignment * floor(rect.XMost() / marginsData->mAlignment); > + float bottom = > + marginsData->mAlignment * floor(rect.YMost() / marginsData->mAlignment); Probably want s/floor/ceil/ for the right and bottom values?

Comment on attachment 8396317 [details] [diff] [review] set the display port base rect Review of attachment 8396317 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsLayoutUtils.cpp @@ +724,5 @@ > return true; > } > > +static void DestroyNsRect(void* aObject, nsIAtom* aPropertyName, > + void* aPropertyValue, void* aData) Won't need this function either with bug 983285. @@ +2311,5 @@ > } > } > > + if (usingDisplayPort && (aFlags & PAINT_WIDGET_LAYERS)) { > + visibleRegion = displayport; nit: trailing whitespace

Comment on attachment 8396319 [details] [diff] [review] set the display port in layer margins Review of attachment 8396319 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsSubDocumentFrame.cpp @@ +386,5 @@ > haveDisplayPort = true; > + // for root content documents we want the base to be the composition bounds > + nsLayoutUtils::SetDisplayPortBase(rootScrollFrame->GetContent(), > + presContext->IsRootContentDocument() ? > + CalculateCompositionSizeForFrame(nsRect(nsPoint(0,0), rootScrollFrame) : missed qref? CalculateCompositionSizeForFrame is in nsLayoutUtils and has a different signature. ::: widget/xpwidgets/APZCCallbackHelper.cpp @@ +93,5 @@ > + aFrameMetrics.LayersPixelsPerCSSPixel(); > + LayerMargin margins = aFrameMetrics.GetDisplayPortMargins(); > + margins.left -= shift.x; > + margins.right += shift.x; > + margins.top -= shift.y; I don't know if this makes sense. The margins are relative anyway so I don't think they need to be shifted at all. If anything it's the base rect that would need to be shifted? @@ +244,5 @@ > + nsRect base(baseCSS.x * nsPresContext::AppUnitsPerCSSPixel(), > + baseCSS.y * nsPresContext::AppUnitsPerCSSPixel(), > + baseCSS.width * nsPresContext::AppUnitsPerCSSPixel(), > + baseCSS.height * nsPresContext::AppUnitsPerCSSPixel()); > + nsLayoutUtils::SetDisplayPortBase(content, base); Might be better to change SetDisplayPortBase to take CSS pixels

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #63) > ::: widget/xpwidgets/APZCCallbackHelper.cpp > @@ +93,5 @@ > > + aFrameMetrics.LayersPixelsPerCSSPixel(); > > + LayerMargin margins = aFrameMetrics.GetDisplayPortMargins(); > > + margins.left -= shift.x; > > + margins.right += shift.x; > > + margins.top -= shift.y; > > I don't know if this makes sense. The margins are relative anyway so I don't > think they need to be shifted at all. If anything it's the base rect that > would need to be shifted? I think it's mathematically the same thing as what we do with rect representation of display port. For example if we have a 100x100 scrollable element (all of it visible) scrolled 1000 pixels down and let's say the difference between scroll offsets is 10, and we have a display port that has 100 extra pixels above and below. With a rect display port we start with (0,-100)x(100,300) ((x,y)x(w,h)), shift by 10 to get (0,-90)x(100,300). With margins we have (100,0,100,0) ((t,r,b,l)) which applied to a 100x100 base rect give the same before rect. Shift by 10 the margins are now (90,0,110,0), applied to the same base rect we now get (0,-90)x(100,300). I don't think it makes sense to shift the base rect. The base rect is the visible part of the scrollable element, and it doesn't change when the element is scrolled.

Ok, makes sense

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #63) > Might be better to change SetDisplayPortBase to take CSS pixels Several other call sites have app units handy. So it's a tossup. Fixed all your other comments.

These will be used later, but I separated them out.

Comment on attachment 8397156 [details] [diff] [review] Part 2, add and implement new layer margins api for display port. Review of attachment 8397156 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsDOMWindowUtils.cpp @@ +423,5 @@ > + // LayerMargin constructor. > + LayerMargin displayportMargins(aTopMargin, > + aRightMargin, > + aBottomMargin, > + aLeftMargin); Doesn't really matter but you can move this down to just before it's used. @@ +451,5 @@ > + nsINode::DeleteProperty<DisplayPortMarginsPropertyData>); > + > + nsIFrame* rootScrollFrame = presShell->GetRootScrollFrame(); > + if (rootScrollFrame) { > + if (content == rootScrollFrame->GetContent()) { Combine the two if conditions with && ::: layout/base/nsLayoutUtils.cpp @@ +699,5 @@ > + gfxSize res = presContext->PresShell()->GetCumulativeResolution(); > + gfxSize parentRes = res; > + if (isRoot) { > + gfxSize localRes = presContext->PresShell()->GetResolution(); > + parentRes.width /= localRes.width; Add a comment as to why this needs to be divided out.

Comment on attachment 8397183 [details] [diff] [review] Part 4, compute and set display ports in layer margins. Review of attachment 8397183 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/xpwidgets/APZCCallbackHelper.cpp @@ +95,5 @@ > + margins.left -= shift.x; > + margins.right += shift.x; > + margins.top -= shift.y; > + margins.bottom += shift.y; > + aFrameMetrics.SetDisplayPortMargins(margins); nit: trailing ws @@ +109,5 @@ > + aFrameMetrics.mDisplayPort.y = (compositionBounds.height - aFrameMetrics.mDisplayPort.height) / 2; > + } else { > + LayerMargin margins = aFrameMetrics.GetDisplayPortMargins(); > + margins.right = margins.left = margins.LeftRight()/2; > + margins.top = margins.bottom = margins.TopBottom()/2; nit: spaces around '/'

I pushed the changes from this bug (with my nits fixed) and bug 986413 to try: https://tbpl.mozilla.org/?tree=Try&rev=0992f3e840bb

Now with build fix for Android (goes in part 1 of this bug's patches): https://tbpl.mozilla.org/?tree=Try&rev=2fec4ac13ac7

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #71) > > + // LayerMargin constructor. > > + LayerMargin displayportMargins(aTopMargin, > > + aRightMargin, > > + aBottomMargin, > > + aLeftMargin); > > Doesn't really matter but you can move this down to just before it's used. > > @@ +451,5 @@ > > + nsINode::DeleteProperty<DisplayPortMarginsPropertyData>); > > + > > + nsIFrame* rootScrollFrame = presShell->GetRootScrollFrame(); > > + if (rootScrollFrame) { > > + if (content == rootScrollFrame->GetContent()) { > > Combine the two if conditions with && The regular displayport rect setter has these same problems, which is why I didn't fix them, just kept them in sync. It would have made the patch less clear. A separate cleanup patch would be the ideal way. I'll post a part 2.1.

Comment on attachment 8397179 [details] [diff] [review] Part 3, set display port base rect in layout when painting. Review of attachment 8397179 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsLayoutUtils.cpp @@ +763,5 @@ > +void > +nsLayoutUtils::SetDisplayPortBase(nsIContent* aContent, const nsRect& aBase) > +{ > + aContent->SetProperty(nsGkAtoms::DisplayPortBase, new nsRect(aBase), > + nsINode::DeleteProperty<nsRect>); trailing whitespace ::: layout/generic/nsSubDocumentFrame.cpp @@ +386,4 @@ > haveDisplayPort = true; > + // for root content documents we want the base to be the composition bounds > + nsLayoutUtils::SetDisplayPortBase(rootScrollFrame->GetContent(), > + presContext->IsRootContentDocument() ? trailing whitespace

Landed on inbound: remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/40651e2d3cbc remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/8b0da7b8afc6 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/4a75c46797d1 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/f9aee922c83b remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/77ca7f1eda03

Added the comment kats asked for https://hg.mozilla.org/integration/mozilla-inbound/rev/b0dbbb5d770a

And apparently we can get a null frame in GetDisplayPort, which seems a little odd, so I'm going to look into it more, but for now I pushed a null check to fix Cipc https://hg.mozilla.org/integration/mozilla-inbound/rev/ed1595ba411b

https://hg.mozilla.org/mozilla-central/rev/40651e2d3cbc https://hg.mozilla.org/mozilla-central/rev/8b0da7b8afc6 https://hg.mozilla.org/mozilla-central/rev/4a75c46797d1 https://hg.mozilla.org/mozilla-central/rev/f9aee922c83b https://hg.mozilla.org/mozilla-central/rev/77ca7f1eda03 https://hg.mozilla.org/mozilla-central/rev/b0dbbb5d770a https://hg.mozilla.org/mozilla-central/rev/ed1595ba411b

(In reply to Timothy Nikkel (:tn) from comment #80) > And apparently we can get a null frame in GetDisplayPort, which seems a > little odd, so I'm going to look into it more, but for now I pushed a null > check to fix Cipc > https://hg.mozilla.org/integration/mozilla-inbound/rev/ed1595ba411b This was a unique corner case. Normally we'd construct a scroll frame (with content the root element), then a canvas frame as a child (with content the root element), and then finally the primary frame of the root element, whether it's a block or other. If the root element is display none or an invalid root svg node then we don't construct this last frame. Leaving us with two frames for the root element, none of hem being the primary frame for it. The fix I landed should be sufficient for this case.

No-Jun, can you test this on the "second" B2G build today, it should have it.

This uplift requires more rebasing for Aurora than I'm comfortable doing.

https://hg.mozilla.org/releases/mozilla-aurora/rev/80984c5e9b3a https://hg.mozilla.org/releases/mozilla-aurora/rev/156c60792f97 https://hg.mozilla.org/releases/mozilla-aurora/rev/b730b23743d3 https://hg.mozilla.org/releases/mozilla-aurora/rev/acfec8a966e3 https://hg.mozilla.org/releases/mozilla-aurora/rev/1e6ec67b5d99

I was testing following master branch on buri, and tried following STR for 947523, and I got "Well this is embarrasing" message of "the page is not responding" from firefox browser. Not sure whether this is related to this fix though. I visited other sites with iframes and scrollable divs such as groupon.com and http://nunzioweb.com/iframes-example.htm, but did not see any painting issues. Gaia 4cf7ec04b82320a1ff1f825220204f454f951a87 Gecko https://hg.mozilla.org/mozilla-central/rev/1c23863d8dda BuildID 20140330160201 Version 31.0a1 Repro Steps: 1) Updated Buri to Build ID: 20131206040203 2) Tap on the "Browser" icon. 3) Navigate to https://etherpad.mozilla.org 4) Tap on "Create new public pad". 5) Keep zooming in on the webpage as far as the user can go. If the crash doesn't occur keeping zooming in on the webpage while changing the phone's orientation. Logcat attached below

When the error message is shown on the phone, the logcat shows following message: I/Gecko ( 136): ###!!! [Parent][MessageChannel] Error: Channel error: cannot send/recv I/Gecko ( 136):

(continue from comment 88) I/Gecko ( 136): ###!!! [Parent][MessageChannel] Error: Channel error: cannot send/recv I/Gecko ( 136): E/OomLogger( 136): [Kill]: select 977 (Browser), adj 2, size 21334, to kill E/OomLogger( 136): [Kill]: send sigkill to 977 (Browser), adj 2, size 21334 E/QCALOG ( 194): [MessageQ] ProcessNewMessage: [XTWiFi-PE] unknown deliver target [OS-Agent] E/QCALOG ( 194): [MessageQ] ProcessNewMessage: [XTWWAN-PE] unknown deliver target [OS-Agent] E/QCALOG ( 194): [MessageQ] ProcessNewMessage: [XT-CS] unknown deliver target [OS-Agent] I/GonkMemoryPressure( 989): Checking to see if memory pressure is over. I/GonkMemoryPressure( 989): Memory pressure is over. I/GonkMemoryPressure( 1051): Checking to see if memory pressure is over. I/GonkMemoryPressure( 1051): Memory pressure is over. I/GonkMemoryPressure( 136): Checking to see if memory pressure is over. I/GonkMemoryPressure( 136): Memory pressure is over. I/Diag_Lib( 139): rpc_handle_rpc_call: for Xid: 2fe, Prog: 31000000, Vers: d17ed9ea, Proc: 00000014 I/ONCRPC ( 139): oncrpc_proxy_handle_cmd_rpc_call: Dispatching xid: 2fe I/Diag_Lib( 139): rpc_handle_rpc_call: Find Status: 0 Xid: 2fe D/QCRIL_RPC( 139): Enter qcril_cm_srvsys_event_callback D/QCRIL_RPC( 139): Exit qcril_cm_srvsys_event_callback I/ONCRPC ( 139): oncrpc_xdr_reply_msg_start: Prog: 00000000, Ver: 00000000, Proc: 00000000 Xid: 000002fe I/ONCRPC ( 139): oncrpc_msg_reply: Prog: 00000000, Ver: 00000000, Proc: 00000000 Xid: 000002fe I/ONCRPC ( 139): oncrpc_proxy_handle_cmd_rpc_call: Dispatch returned for xid: 2fe I/Gecko ( 136): I/Gecko ( 136): ###!!! [Parent][MessageChannel] Error: Channel error: cannot send/recv I/Gecko ( 136): E/QCALOG ( 194): [MessageQ] ProcessNewMessage: [XTWiFi-PE] unknown deliver target [OS-Agent] E/QCALOG ( 194): [MessageQ] ProcessNewMessage: [XT-CS] unknown deliver target [OS-Agent] E/QCALOG ( 194): [MessageQ] ProcessNewMessage: [XTWWAN-PE] unknown deliver target [OS-Agent] I/Gonk ( 136): Setting nice for pid 1051 to 18 I/Gonk ( 136): Changed nice for pid 1051 from 18 to 18. I/Gecko ( 1051): ###################################### forms.js loaded I/Gecko ( 1051): ############################### browserElementPanning.js loaded I/Gecko ( 1051): ######################## BrowserElementChildPreload.js loaded E/memalloc( 136): /dev/pmem: No more pmem available W/memalloc( 136): Falling back to ashmem D/skia ( 1051): START /proc/cpuinfo: D/skia ( 1051): Processor : ARMv7 Processor rev 1 (v7l) D/skia ( 1051): processor : 0 D/skia ( 1051): BogoMIPS : 501.64 D/skia ( 1051): D/skia ( 1051): Features : swp half thumb fastmult vfp edsp thumbee neon vfpv3 tls vfpv4 D/skia ( 1051): CPU implementer : 0x41 D/skia ( 1051): CPU architecture: 7 D/skia ( 1051): CPU variant : 0x0 D/skia ( 1051): CPU part : 0xc05 D/skia ( 1051): CPU revision : 1 D/skia ( 1051): D/skia ( 1051): Hardware : QCT MSM7x27a FFA D/skia ( 1051): Revision : 0000 D/skia ( 1051): Serial : 0000000000000000 D/skia ( 1051): D/skia ( 1051): END /proc/cpuinfo D/skia ( 1051): Device supports ARM NEON instructions! I/Gecko ( 136): I/Gecko ( 136): ###!!! [Parent][MessageChannel] Error: Channel error: cannot send/recv I/Gecko ( 136): E/msm7627a.hwcomposer( 136): drawLayerUsingCopybit: greater than max supported size dsdx=0.003125 dtdy=320.000000 scaleLimitMax=16.000000 scaleLimitMin=0.062500 E/msm7627a.hwcomposer( 136): hwc_set: Copybit layer draw failed! E/msm7627a.hwcomposer( 136): hwc_set: Unable to render by hwc due to non-pmem memory E/msm7627a.hwcomposer( 136): drawLayerUsingCopybit: greater than max supported size dsdx=0.003125 dtdy=320.000000 scaleLimitMax=16.000000 scaleLimitMin=0.062500 E/msm7627a.hwcomposer( 136): hwc_set: Copybit layer draw failed! E/msm7627a.hwcomposer( 136): hwc_set: Unable to render by hwc due to non-pmem memory E/msm7627a.hwcomposer( 136): drawLayerUsingCopybit: greater than max supported size dsdx=0.003125 dtdy=320.000000 scaleLimitMax=16.000000 scaleLimitMin=0.062500 E/msm7627a.hwcomposer( 136): hwc_set: Copybit layer draw failed! E/msm7627a.hwcomposer( 136): hwc_set: Unable to render by hwc due to non-pmem memory E/msm7627a.hwcomposer( 136): drawLayerUsingCopybit: greater than max supported size dsdx=0.003125 dtdy=320.000000 scaleLimitMax=16.000000 scaleLimitMin=0.062500 E/msm7627a.hwcomposer( 136): hwc_set: Copybit layer draw failed! E/msm7627a.hwcomposer( 136): hwc_set: Unable to render by hwc due to non-pmem memory E/msm7627a.hwcomposer( 136): drawLayerUsingCopybit: greater than max supported size dsdx=0.003125 dtdy=320.000000 scaleLimitMax=16.000000 scaleLimitMin=0.062500 E/msm7627a.hwcomposer( 136): hwc_set: Copybit layer draw failed! E/Profiler( 136): BPUnw: [1 total] thread_unregister_for_profiling(me=0x1cea2d0) (NOT REGISTERED) E/memalloc( 136): /dev/pmem: No more pmem available W/memalloc( 136): Falling back to ashmem E/memalloc( 136): /dev/pmem: No more pmem available W/memalloc( 136): Falling back to ashmem E/memalloc( 136): /dev/pmem: No more pmem available W/memalloc( 136): Falling back to ashmem E/memalloc( 136): /dev/pmem: No more pmem available W/memalloc( 136): Falling back to ashmem E/memalloc( 136): /dev/pmem: No more pmem available W/memalloc( 136): Falling back to ashmem

tn, can you see if you can repro this and figure out what's going on? Probably worth forking that to a separate bug.

I can repro, I filed bug 989897.