1121198 - Cleanup some usages of nsDocShell::GetPositionAndSize()

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

I noticed a few things that can be cleaned up w.r.t. nsDocShell::GetPositionAndSize():

 (1) Some callers call it & pass in nullptr for either the position args or the size args. These callers should just use the GetPosition() / GetSize() accessors.

 (2) The GetPosition() accessor currently passes in a throwaway variable for the size args.  However, ever since bug 519590 (mozilla-central changeset e9d59855daf9), this makes GetPositionAndSize() slower (because it flushes reflow if it thinks the caller wants the size). So, GetPosition() should just pass in nullptr for the position args.

 (3) Some callers instantiate a nsWeakFrame, and check whether it's still alive after calling GetPositionAndSize(), in case there was a layout flush. But in cases where the caller doesn't actually ask for the size*, we know there won't be a layout flush, so this nsWeakFrame is unnecessary.  So, I think we should remove it.

* In particular, nsFrameLoader::UpdateBaseWindowPositionAndSize and its (indirect) caller nsSubDocumentFrame::ReflowFinished() both create a weakFrame and check it, with comment "GetPositionAndSize() killed us" in the former case.  This dates back to bug 399410 comment 12, and it was needed at that point, but it hasn't been needed since bug 519590 as described above.

Comment 1

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #0)
>  (1) Some callers call it & pass in nullptr for either the position args or
> the size args. These callers should just use the GetPosition() / GetSize()
> accessors.

(Er, I guess there's really just one caller that does this -- the one in nsFrameLoader::UpdateBaseWindowPositionAndSize.  I thought I'd found others in nsXULWindow, but those are calls to that class's own GetPositionAndSize method, not direct calls to nsDocShell's method.)

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: part 1: Make nsDocShell GetPosition/GetSize pass null outparams for unwanted info.

(This is labeled as "(2)" in comment 0, but it actually makes the most sense at the beginning of the patch queue here, so I'm posting it as "part 1")

Comment 3

[Daniel Holbert [:dholbert]]

FWIW, here's a link to the GetPositionAndSize impl to demonstrate that part 1 should have no effect, because its args are null-checked before they're used:
https://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp?rev=6a2b37d4c1e2&mark=5845-5883#5845

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: part 2: simply nsFrameLoader with s/GetPositionAndSize()/GetPosition()/

This simplifies a GetPositionAndSize() call to just GetPosition().

(I called this (1) in comment 0, but it makes more sense at this point in the patch queue, after the previous patch's GetPosition() simplifications, to make it pass nullptr for the unused params.)

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: part [UNWANTED]: remove unneeded nsWeakFrames

This patch handles cleanup (3) from comment 0 -- it removes two nsWeakFrames that are present out of fear that GetPositionAndSize() might flush layout and destroy some frames.

Given that we're explicitly *not* getting the size, we can be sure that it *won't* flush layout, so we don't need these checks.

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: part 3: Make nsFrameLoader::UpdateBaseWindowPositionAndSize infallible

One final bit of cleanup: I noticed nsFrameLoader::UpdateBaseWindowPositionAndSize is guaranteed to return NS_OK (previously via two paths, now via one path).  So I'm just changing its return type to 'void', to simplify things.

Comment 7

[Daniel Holbert [:dholbert]]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3383a436b010

Comment 8

[Daniel Holbert [:dholbert]]

Hmm, so the Try run shows a debug build abort due to "PresArena: poison overwritten", while in NS_NewSubDocumentFrame.  So, something's wrong, though I'm not sure what yet. (perhaps we do need the weak frame checks after all?)

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8548491 [details] [diff] [review]
part 1: Make nsDocShell GetPosition/GetSize pass null outparams for unwanted info.

r=me

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8548494 [details] [diff] [review]
part 2: simply nsFrameLoader with s/GetPositionAndSize()/GetPosition()/

r=me on the GetPosition bit, but the comment change is wrong.

Comment 11

[Daniel Holbert [:dholbert]]

(Given that the comment change is wrong, I suspect part 3 will be r- then. FWIW, I can reproduce comment 8's abort locally, if part 3 is applied, though I'm not yet clear on what's going on.)

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8548498 [details] [diff] [review]
part [UNWANTED]: remove unneeded nsWeakFrames

The reason this is failing is a bit subtle.

nsDocShell::GetPositionAndSize will call SetPositionAndSize in the mParentWidget case.  This will call nsDocumentViewer::SetBounds which will call nsViewManager::SetWindowDimensions which will end up doing a PresShell::ResizeReflow from inside nsViewManager::DoSetWindowDimensions.  

ResizeReflow will synchronously do a reflow, and in the process flush styles, etc.  So it can in fact destroy frames.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8548500 [details] [diff] [review]
part 3: Make nsFrameLoader::UpdateBaseWindowPositionAndSize infallible

r=me

Comment 14

[Boris Zbarsky [:bzbarsky]]

I mean, maybe there's something else going on other than the codepath in comment 12, but the codepath in comment 12 is certainly _a_ way GetPositionAndSize can end up destroying frames.

Comment 15

[Daniel Holbert [:dholbert]]

Thanks -- yeah, I've confirmed that we do indeed hit that exact codepath that you mentioned in comment 12, during this devtools mochitest.

Here's a partial backtrace (with patches applied up to "part 3"):
(gdb) bt
#0  PresShell::ResizeReflow (this=0x7fbc5d407c00, aWidth=86400, 
    aHeight=48060)
    at $SRC/layout/base/nsPresShell.cpp:2023
#1  0x00007fbc8d70eaf2 in nsViewManager::DoSetWindowDimensions (
    this=0x7fbc5d43ab80, aWidth=86400, aHeight=48060)
    at $SRC/view/nsViewManager.cpp:192
#2  0x00007fbc8d70ec5a in nsViewManager::SetWindowDimensions (
    this=0x7fbc5d43ab80, aWidth=86400, aHeight=48060)
    at $SRC/view/nsViewManager.cpp:212
#3  0x00007fbc8db6d00d in nsDocumentViewer::SetBounds (
    this=0x7fbc64e21680, aBounds=...)
    at $SRC/layout/base/nsDocumentViewer.cpp:1943
#4  0x00007fbc8df24f0e in nsDocShell::SetPositionAndSize (
    this=0x7fbc61e1a000, x=0, y=0, cx=1440, cy=801, fRepaint=false)
    at $SRC/docshell/base/nsDocShell.cpp:5837
#5  0x00007fbc8df25a77 in non-virtual thunk to nsDocShell::SetPositionAndSize(int, int, int, int, bool) ()
    at $SRC/docshell/base/nsDocShell.cpp:5841
#6  0x00007fbc8bdf6f95 in nsFrameLoader::UpdateBaseWindowPositionAndSize (this=0x7fbc75ba42e0, aIFrame=0x7fbc6202bed0)
    at $SRC/dom/base/nsFrameLoader.cpp:1984
#7  0x00007fbc8bdf6e78 in nsFrameLoader::UpdatePositionAndSize (
    this=0x7fbc75ba42e0, aIFrame=0x7fbc6202bed0)
    at $SRC/dom/base/nsFrameLoader.cpp:1966
#8  0x00007fbc8dd08c1f in nsSubDocumentFrame::ReflowFinished (
    this=0x7fbc6202bed0)

Comment 16

[Daniel Holbert [:dholbert]]

So that means in bug 1110950 part 3 (attachment 8548698 [details] [diff] [review]), I probably need to make my "mFrameLoader->UpdatePositionAndSize()" call happen asynchronously.  Right now, it's triggered off of a style change. (adding a new nsChangeHint)

Comment 17

[Boris Zbarsky [:bzbarsky]]

Or we could make ResizeReflow be async, maybe?  I'm not sure it really needs to be sync...

Comment 18

[Daniel Holbert [:dholbert]]

That's intriguing, though it sounds like it could be a bit involved to get that working correctly. (In particular, we probably don't want to block bug 1110950 on making that change.)

Comment 19

[Daniel Holbert [:dholbert]]

Comment on attachment 8548498 [details] [diff] [review]
part [UNWANTED]: remove unneeded nsWeakFrames

(bookkeeping note: for sane-hg-history's sake, I'm going to rename "part 4" here to "part 3", and mark the former "part 3" as unwanted -- so that we don't end up with a mysteriously missing part-number, in the push for this bug.)

Comment 20

[Daniel Holbert [:dholbert]]

Landed:
part 1: https://hg.mozilla.org/integration/mozilla-inbound/rev/c542abcd839e
part 2: https://hg.mozilla.org/integration/mozilla-inbound/rev/5fa4d830c36d
part 3 (formerly named part 4): https://hg.mozilla.org/integration/mozilla-inbound/rev/c834d4aeee3e

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c542abcd839e
https://hg.mozilla.org/mozilla-central/rev/5fa4d830c36d
https://hg.mozilla.org/mozilla-central/rev/c834d4aeee3e