Closed Bug 1121198 Opened 5 years ago Closed 5 years ago

Cleanup some usages of nsDocShell::GetPositionAndSize()

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

Attachments

(4 files)

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.
(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.)
(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")
Attachment #8548491 - Flags: review?(bzbarsky)
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
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.)
Attachment #8548494 - Flags: review?(bzbarsky)
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.
Attachment #8548498 - Flags: review?(bzbarsky)
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.
Attachment #8548500 - Flags: review?(bzbarsky)
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 on attachment 8548491 [details] [diff] [review]
part 1: Make nsDocShell GetPosition/GetSize pass null outparams for unwanted info.

r=me
Attachment #8548491 - Flags: review?(bzbarsky) → review+
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.
Attachment #8548494 - Flags: review?(bzbarsky) → review+
(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 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.
Attachment #8548498 - Flags: review?(bzbarsky) → review-
Comment on attachment 8548500 [details] [diff] [review]
part 3: Make nsFrameLoader::UpdateBaseWindowPositionAndSize infallible

r=me
Attachment #8548500 - Flags: review?(bzbarsky) → review+
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.
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)
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)
Or we could make ResizeReflow be async, maybe?  I'm not sure it really needs to be sync...
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 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.)
Attachment #8548498 - Attachment description: part 3: remove unneeded nsWeakFrames → part [UNWANTED]: remove unneeded nsWeakFrames
Attachment #8548500 - Attachment description: part 4: Make nsFrameLoader::UpdateBaseWindowPositionAndSize infallible → part 3: Make nsFrameLoader::UpdateBaseWindowPositionAndSize infallible
You need to log in before you can comment on or make changes to this bug.