PresShell::mSelection is being set for SVG-as-an-image and is wasting 1504 B on OS X. On the keon, contacts.svg in the benchmark in bug 999931 takes around 50 KB. I've not checked what the size of mSelection is on the keon, but it will certainly be big enough that we want to avoid it.

diff -w version

Comment on attachment 8752071 [details] [diff] [review] 1016673.txt Review of attachment 8752071 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsPresShell.cpp @@ +901,5 @@ > // Need to happen before nsFrameSelection has been set up. > mAccessibleCaretEventHub = new AccessibleCaretEventHub(this); > } > > + if (!aDocument->IsBeingUsedAsImage()) { This should probably be a check for "IsResourceDoc()" (which covers both SVG images as well as helper documents that we reference for filters/<use>/etc.) (Same goes for the similar code in nsDocumentViewer.cpp.) @@ +903,5 @@ > } > > + if (!aDocument->IsBeingUsedAsImage()) { > + > + mSelection = new nsFrameSelection(); So now we're allowing nsIPresShell::mSelection to be null (which I guess is the point of this bug). That means we need to rename the nsIPresShell::ConstFrameSelection() method (which just returns this member) -- it needs a "Get" prefix now, to indicate that it can return nullptr. Also, we need to audit the callers of that method, to be sure they either can't be called for resource-docs, or that they're OK with a null return-value.

(Here's a link to that getter which needs renaming/caller-auditing: http://mxr.mozilla.org/mozilla-central/source/layout/base/nsIPresShell.h?rev=b02eddb72e69#398 )

(The "ConstFrameSelection" rename & its null-handling tweaks would probably want to happen in a separate patch, probably as a "part 0" here, to pave the way for this element actually being sometimes-null before we *make* it sometimes null.)

How about keeping the name of nsIPresShell::ConstFrameSelection as is but inside it asserting that IsResourceDoc() is false?

I'm not sure I'd be on board with that... In particular: (1) It would still be breaking our "getter-function" naming guidelines. (In an opt build, it would still be able to return something null, so it's misleading to have it be named something that implies it always returns non-null.) The point is: callers can't just blindly use it and assume they're OK to do so. An assertion would *help* catch/prevent such callers, but I fully expect some cases to fall through the cracks & not get caught via testing (since resource/image-documents are relatively rare). (2) Some calls to ConstFrameSelection() are just checking it for equality against some other value -- not actually *using* the return-value. So: if some of those calls happen to get invoked for a resource document, that seems like it'd be harmless and fine -- which means spamming an assertion-failure may be overkill.

To feel more confident about whether the rename is warranted/not-warranted, I think we'd need to audit the callers and get a better idea of how impossible it is for them to be called for a resource document (and hence return null). For any callers that *assume* the return-value is non-null (i.e. callers that aren't just doing an equality check), we probably would benefit from adding a code-comment to justify why their assumption is valid [how we know they're working with a non-resource doc], even if that just means "add an explicit IsResourceDoc check, because we're not sure". And at that point, I think a "Get" rename is merited. As for assertions, I think it *would* be valid to make GetConstFrameSelection() internally assert that mSelection is non-null iff IsResourceDoc() is true, though. That would make potential IsResourceDoc() checks in the callers make a bit more sense.

(At the moment, I'm punting on auditing/trying-to-understand the callers myself, as I'm catching up on reviews/bugmail after a week of css working-group meetings and don't have time to deep-dive. I'm hoping you might be up for doing that, as a requisite part of this bug. :) But if not, I may be able to get to it later on.)

Comment on attachment 8752399 [details] [diff] [review] Part 0 - rename ConstFrameSelection Review of attachment 8752399 [details] [diff] [review]: ----------------------------------------------------------------- This isn't quite done. Most of the API callsites here still blindly use the returned object, without any sort of null-check or justification for why they think the thing is non-null. This was reasonable of them to do, up until this point, because the API was guaranteed to return non-null. But it is no longer reasonable, as of this change (which makes the API fallible). We need to fix the callsites so that they all either check for failure, or justify why they know it can't fail. (except for the callsites that clearly don't care whether the returned thing is null, like the one that just compares the returned thing for equality). This is the audit that I was hinting at above. I don't think we can take a fix for this bug until we've done this audit and fixed the callsites to either null-check or declare why they're justified in assuming the returned thing is nonnull. ::: layout/base/nsIPresShell.h @@ +394,5 @@ > */ > already_AddRefed<nsFrameSelection> FrameSelection(); > > /** > * ConstFrameSelection returns an object which methods are safe to use for This documentation still has a mention of the old function-name & needs a rename. (Feel free to just replace "ConstFrameSelection" with "This function" here if you like.) And while you're touching this line, please s/which methods/whose methods/ for readability.

Comment on attachment 8752446 [details] [diff] [review] Part 1 (using IsResourceDoc) Review of attachment 8752446 [details] [diff] [review]: ----------------------------------------------------------------- r=me on part 1, once we've got part 0 settled. Just two nits below: ::: layout/base/nsPresShell.cpp @@ +903,5 @@ > } > > + if (!aDocument->IsResourceDoc()) { > + > + mSelection = new nsFrameSelection(); Drop the blank line just inside the "if" check here. @@ +921,5 @@ > + nsPresContext::nsPresContextType type = aPresContext->Type(); > + if (type != nsPresContext::eContext_PrintPreview && > + type != nsPresContext::eContext_Print) > + SetDisplaySelection(nsISelectionController::SELECTION_DISABLED); > + } This closing curly-brace that you're adding makes the existing un-braced "if" check here somewhat confusing to visually parse. So: please add curly-braces around this nested "if" check, so that the closing curly-brace at the end of this block doesn't end up being visually ambiguous.

(In reply to Daniel Holbert [:dholbert] from comment #13) > Comment on attachment 8752399 [details] [diff] [review] > Part 0 - rename ConstFrameSelection > > Review of attachment 8752399 [details] [diff] [review]: > ----------------------------------------------------------------- > > This isn't quite done. Most of the API callsites here still blindly use the > returned object, without any sort of null-check or justification for why > they think the thing is non-null. > The methods are never used by images. I'd rather just assert that the callers are not working with a ResourceDoc

The accessible code file seems to only use NS_ASSERTION which is why I've stuck with that in that file.

Comment on attachment 8753128 [details] [diff] [review] Part 0 with asserts Review of attachment 8753128 [details] [diff] [review]: ----------------------------------------------------------------- Sorry, this still isn't quite r+ (though not through any fault of yours; I'm just realizing the rabbit hole goes a bit deeper. See note about FrameSelection() below.) ::: accessible/base/SelectionManager.cpp @@ +100,5 @@ > > void > SelectionManager::AddDocSelectionListener(nsIPresShell* aPresShell) > { > + NS_ASSERTION(!aPresShell->GetDocument()->IsResourceDoc(), "Caller should not be a ResourceDoc"); Please wrap after the comma here, so this line isn't quite so long. This applies to every instance of this assertion. ::: accessible/html/HTMLTableAccessible.cpp @@ +801,5 @@ > else > count = RowCount(); > > nsIPresShell* presShell(mDoc->PresShell()); > + RefPtr<nsFrameSelection> tableSelection = presShell->FrameSelection(); Hmm. Looks like the function you're swapping in here (FrameSelection()) needs a rename and some assertions as well, since it's just doing the same thing as the method-formerly-known-as-ConstFrameSelection() -- it's returning a pointer to mSelection (which is newly allowed to be null in this bug). It looks like this method has 12 callers. :-/ We probably need to fix those to have assertions as well, for any that actually use the returned value (and add an assertion here, too). @@ +826,5 @@ > if (!tableFrame) > return NS_OK; > > nsIPresShell* presShell(mDoc->PresShell()); > + RefPtr<nsFrameSelection> tableSelection = presShell->FrameSelection(); As above, this one will needs an assertion (as this function needs a rename, too). ::: layout/base/nsIPresShell.h @@ +394,5 @@ > */ > already_AddRefed<nsFrameSelection> FrameSelection(); > > /** > + * This method returns an object whose methods are safe to use for Two concerns: (1) I wonder if this "an object whose methods are safe to use" thing is even meaningful anymore? I have no idea what it's actually saying. I wonder if this predated the "You cannot go back and forth anymore" comment which FrameSelection() now has in its documentation. So: maybe this "safe to use" language wants to be dropped? Though I guess I'm fine defaulting to leaving it there, in the absence of an understanding of what it means. (2) We need to update this documentation to indicate that this method is guaranteed to return non-null, *except for* resource documents, where it may (or will) return null. (Same goes for FrameSelection() above, too.) With that documented, the assertions you're adding (about IsResourceDocument) will make more sense.

(In reply to Daniel Holbert [:dholbert] from comment #20) > Two concerns: > (1) I wonder if this "an object whose methods are safe to use" thing is > even meaningful anymore? I have no idea what it's actually saying. I wonder > if this predated the "You cannot go back and forth anymore" comment which > FrameSelection() now has in its documentation. (In particular, I wonder if this comment about "safe to use" became *obsolete* as soon as that "cannot...anymore" comment was added to FrameSelection()? Not sure.)

Comment on attachment 8755130 [details] [diff] [review] part 0 again Review of attachment 8755130 [details] [diff] [review]: ----------------------------------------------------------------- So, ultimately this probably needs review from someone who knows more about these call-sites than I do (or at least has some say over DOM and/or a11y code), because this is the first time I've looked at most of these files, so 'm not sure offhand if the changes are sound. Maybe smaug? (Sorry this has turned into a rabbit hole. :-/ It was somewhat doomed to a bit of rabbit-holing from the start, given that the relatively-simple change is really relaxing the guarantees around some APIs which have callers across the tree.) In the meantime, here's my feedback on this latest patch: ::: accessible/base/SelectionManager.cpp @@ +101,5 @@ > void > SelectionManager::AddDocSelectionListener(nsIPresShell* aPresShell) > { > + NS_ASSERTION(!aPresShell->GetDocument()->IsResourceDoc(), > + "Caller should not be a ResourceDoc"); Two things: (1) Hmm... So technically, every "GetDocument()->IsResourceDoc()" call inside of an assertion is *assuming* that GetDocument() will return something non-null, with no clear justification. (since "Get" in "GetDocument" is saying "may return null"). And that's bad, because if GetDocument() returns null, then we'll crash when evaluating the asserted condition! (before we can even print out the assertion text) So, for assertions that involve a GetDocument() call, we might need to assert something closer to: aPresShell->GetDocument() && !aPresShell->GetDocument()->IsResourceDoc() ...which will let us fail the assertion instead of crashing, if we happen to hit this code in a scenario where GetDocument() reutrns null. (I don't know offhand how likely such scenarios are, in these callsites. In some cases we can look at surrounding/later code to see if the document is null-checked, but e.g. here we don't have that ability. Intuitively, it seems like we shouldn't be doing work with selections for a presshell that has no document, but I don't know that for sure.) (2) The assertion text here (and throughout this patch) might need some massaging. I think you really want to say "Should not be called for a ResourceDoc"? (In most cases here, the caller is not a document, I think, so "caller should not be a resourcedoc" doesn't really make sense.) ::: layout/base/nsPresShell.cpp @@ +2431,5 @@ > { > + NS_ASSERTION(!GetDocument()->IsResourceDoc(), > + "Caller should not be a ResourceDoc"); > + > + RefPtr<nsFrameSelection> frameSelection = GetFrameSelection(); You don't need the IsResourceDoc() assertion here, because this function null-checks |frameSelection| before using it (just after this quoted block). (I don't know offhand why the null-check is there -- maybe it could go away? And I don't have a good intuition for whether or not this function could conceivably be called for resource docs. So, I tend to lean towards being conservative & leaving the existing null-check in, which means we don't have a strong reason for this assertion.) ::: layout/generic/nsFrame.cpp @@ +3765,5 @@ > // Also check the selection of the capturing content which might be in a > // different document. > if (!frameSelection && captureContent) { > nsIDocument* doc = captureContent->GetUncomposedDoc(); > + NS_ASSERTION(!doc->IsResourceDoc(), "Caller should not be a ResourceDoc"); This line needs to move down one line (after we've null-checked |doc|.) Otherwise we'll crash (in debug builds) while evaluating the assertion, if we get here with a null |doc| (as we apparently can, since there's a null-check). @@ +6433,5 @@ > > const nsFrameSelection* > nsIFrame::GetConstFrameSelection() const > { > + MOZ_ASSERT(!PresContext()->Document()->IsResourceDoc(), "Should not be called with a ResourceDoc"); We don't need this assertion here. This function does not depend on the PresShell "ConstFrameSelection()" API returning something non-null.

(In reply to Daniel Holbert [:dholbert] from comment #23) > So, for assertions that involve a GetDocument() call, we might need to > assert something closer to: > aPresShell->GetDocument() && > !aPresShell->GetDocument()->IsResourceDoc() > ...which will let us fail the assertion instead of crashing, if we happen to > hit this code in a scenario where GetDocument() reutrns null. (Note that I don't know for sure that this ^^ asserted condition is something that we're actually justified in asserting :-/ I'm just suggesting that it's one way to avoid crashing on a null GetDocument() during the assertion. But, it might in fact be better to assert... !aPresShell->GetDocument() || !aPresShell->GetDocument()->IsResourceDoc() ...which would also protect us from the crash (but allow us to pass w/out asserting when GetDocument() returns null). Some callsites might want one of these versions, and some might want the other, potentially...)

Comment on attachment 8755130 [details] [diff] [review] part 0 again Marking this r- for now, to clean up my review queue a little. See comment 23 for latest review feedback/thoughts.