From 17f7e1c8c6fca351174bdbd73981aa8e44d0f9da Mon Sep 17 00:00:00 2001 From: "Matt A. Tobin" Date: Fri, 17 Apr 2020 05:33:06 -0400 Subject: Bug 1365092 - Move side effects of SetAttr and ParseAttribute to BeforeSetAttr and AfterSetAttr * Moves side effects of nsGenericHTMLElement and Element's SetAttr, UnsetAttr, and ParseAttribute functions to the corresponding BeforeSetAttr and AfterSetAttr functions * Moves side effects of HTMLAnchorElement's SetAttr, UnsetAttr, and ParseAttribute functions to the corresponding BeforeSetAttr and AfterSetAttr functions * Moves side effects of HTMLImageElement's SetAttr function to the corresponding BeforeSetAttr and AfterSetAttr functions * Moves side effects of SetAttr, UnsetAttr, and ParseAttribute functions to BeforeSetAttr and AfterSetAttr Tag #1375 --- dom/base/Element.cpp | 94 +++++++++++++++++++++++++++++++----------- dom/base/Element.h | 66 ++++++++++++++++++++++++++--- dom/base/nsAttrValueOrString.h | 14 +++++++ dom/base/nsStyledElement.cpp | 17 +++++++- dom/base/nsStyledElement.h | 4 ++ 5 files changed, 165 insertions(+), 30 deletions(-) (limited to 'dom/base') diff --git a/dom/base/Element.cpp b/dom/base/Element.cpp index 11ad042a0e..96ccfd52a7 100644 --- a/dom/base/Element.cpp +++ b/dom/base/Element.cpp @@ -2476,7 +2476,7 @@ Element::SetAttr(int32_t aNamespaceID, nsIAtom* aName, if (OnlyNotifySameValueSet(aNamespaceID, aName, aPrefix, value, aNotify, oldValue, &modType, &hasListeners, &oldValueSet)) { - return NS_OK; + return OnAttrSetButNotChanged(aNamespaceID, aName, value, aNotify); } nsAttrValue attrValue; @@ -2494,20 +2494,21 @@ Element::SetAttr(int32_t aNamespaceID, nsIAtom* aName, preparsedAttrValue); } - nsresult rv = BeforeSetAttr(aNamespaceID, aName, &value, aNotify); - NS_ENSURE_SUCCESS(rv, rv); - // Hold a script blocker while calling ParseAttribute since that can call // out to id-observers nsIDocument* document = GetComposedDoc(); mozAutoDocUpdate updateBatch(document, UPDATE_CONTENT_MODEL, aNotify); - // Even the value was pre-parsed, we still need to call ParseAttribute because - // it can have side effects. - if (!ParseAttribute(aNamespaceID, aName, aValue, attrValue)) { + nsresult rv = BeforeSetAttr(aNamespaceID, aName, &value, aNotify); + NS_ENSURE_SUCCESS(rv, rv); + + if (!preparsedAttrValue && + !ParseAttribute(aNamespaceID, aName, aValue, attrValue)) { attrValue.SetTo(aValue); } + PreIdMaybeChange(aNamespaceID, aName, &value); + return SetAttrAndNotify(aNamespaceID, aName, aPrefix, oldValueSet ? &oldValue : nullptr, attrValue, modType, hasListeners, aNotify, @@ -2538,7 +2539,7 @@ Element::SetParsedAttr(int32_t aNamespaceID, nsIAtom* aName, if (OnlyNotifySameValueSet(aNamespaceID, aName, aPrefix, value, aNotify, oldValue, &modType, &hasListeners, &oldValueSet)) { - return NS_OK; + return OnAttrSetButNotChanged(aNamespaceID, aName, value, aNotify); } if (aNotify) { @@ -2549,6 +2550,8 @@ Element::SetParsedAttr(int32_t aNamespaceID, nsIAtom* aName, nsresult rv = BeforeSetAttr(aNamespaceID, aName, &value, aNotify); NS_ENSURE_SUCCESS(rv, rv); + PreIdMaybeChange(aNamespaceID, aName, &value); + nsIDocument* document = GetComposedDoc(); mozAutoDocUpdate updateBatch(document, UPDATE_CONTENT_MODEL, aNotify); return SetAttrAndNotify(aNamespaceID, aName, aPrefix, @@ -2608,6 +2611,8 @@ Element::SetAttrAndNotify(int32_t aNamespaceID, } NS_ENSURE_SUCCESS(rv, rv); + PostIdMaybeChange(aNamespaceID, aName, &valueForAfterSetAttr); + // If the old value owns its own data, we know it is OK to keep using it. // oldValue will be null if there was no previously set value const nsAttrValue* oldValue; @@ -2720,22 +2725,16 @@ Element::ParseAttribute(int32_t aNamespaceID, nsAttrValue& aResult) { if (aNamespaceID == kNameSpaceID_None) { - if (aAttribute == nsGkAtoms::_class) { - SetMayHaveClass(); - // Result should have been preparsed above. - return true; - } + MOZ_ASSERT(aAttribute != nsGkAtoms::_class, + "The class attribute should be preparsed and therefore should " + "never be passed to Element::ParseAttribute"); if (aAttribute == nsGkAtoms::id) { // Store id as an atom. id="" means that the element has no id, // not that it has an emptystring as the id. - RemoveFromIdTable(); if (aValue.IsEmpty()) { - ClearHasID(); return false; } aResult.ParseAtom(aValue); - SetHasID(); - AddToIdTable(aResult.GetAtomValue()); return true; } } @@ -2754,6 +2753,57 @@ Element::SetAndSwapMappedAttribute(nsIDocument* aDocument, return false; } +nsresult +Element::BeforeSetAttr(int32_t aNamespaceID, nsIAtom* aName, + const nsAttrValueOrString* aValue, bool aNotify) +{ + if (aNamespaceID == kNameSpaceID_None) { + if (aName == nsGkAtoms::_class) { + if (aValue) { + // Note: This flag is asymmetrical. It is never unset and isn't exact. + // If it is ever made to be exact, we probably need to handle this + // similarly to how ids are handled in PreIdMaybeChange and + // PostIdMaybeChange. + SetMayHaveClass(); + } + } + } + + return NS_OK; +} + +void +Element::PreIdMaybeChange(int32_t aNamespaceID, nsIAtom* aName, + const nsAttrValueOrString* aValue) +{ + if (aNamespaceID != kNameSpaceID_None || aName != nsGkAtoms::id) { + return; + } + RemoveFromIdTable(); + + return; +} + +void +Element::PostIdMaybeChange(int32_t aNamespaceID, nsIAtom* aName, + const nsAttrValue* aValue) +{ + if (aNamespaceID != kNameSpaceID_None || aName != nsGkAtoms::id) { + return; + } + + // id="" means that the element has no id, not that it has an empty + // string as the id. + if (aValue && !aValue->IsEmptyString()) { + SetHasID(); + AddToIdTable(aValue->GetAtomValue()); + } else { + ClearHasID(); + } + + return; +} + EventListenerManager* Element::GetEventListenerManagerForAttr(nsIAtom* aAttrName, bool* aDefer) @@ -2847,6 +2897,8 @@ Element::UnsetAttr(int32_t aNameSpaceID, nsIAtom* aName, NS_EVENT_BITS_MUTATION_ATTRMODIFIED, this); + PreIdMaybeChange(aNameSpaceID, aName, nullptr); + // Grab the attr node if needed before we remove it from the attr map RefPtr attrNode; if (hasMutationListeners) { @@ -2865,12 +2917,6 @@ Element::UnsetAttr(int32_t aNameSpaceID, nsIAtom* aName, // react to unexpected attribute changes. nsMutationGuard::DidMutate(); - if (aName == nsGkAtoms::id && aNameSpaceID == kNameSpaceID_None) { - // Have to do this before clearing flag. See RemoveFromIdTable - RemoveFromIdTable(); - ClearHasID(); - } - bool hadValidDir = false; bool hadDirAuto = false; @@ -2883,6 +2929,8 @@ Element::UnsetAttr(int32_t aNameSpaceID, nsIAtom* aName, rv = mAttrsAndChildren.RemoveAttrAt(index, oldValue); NS_ENSURE_SUCCESS(rv, rv); + PostIdMaybeChange(aNameSpaceID, aName, nullptr); + if (document || HasFlag(NODE_FORCE_XBL_BINDINGS)) { RefPtr binding = GetXBLBinding(); if (binding) { diff --git a/dom/base/Element.h b/dom/base/Element.h index 6f60c9ad0d..df0dbcc45e 100644 --- a/dom/base/Element.h +++ b/dom/base/Element.h @@ -523,6 +523,7 @@ public: already_AddRefed GetExistingAttrNameFromQName(const nsAString& aStr) const; + MOZ_ALWAYS_INLINE // Avoid a crashy hook from Avast 10 Beta (Bug 1058131) nsresult SetAttr(int32_t aNameSpaceID, nsIAtom* aName, const nsAString& aValue, bool aNotify) { @@ -1378,14 +1379,9 @@ protected: * will be null. * @param aNotify Whether we plan to notify document observers. */ - // Note that this is inlined so that when subclasses call it it gets - // inlined. Those calls don't go through a vtable. virtual nsresult BeforeSetAttr(int32_t aNamespaceID, nsIAtom* aName, const nsAttrValueOrString* aValue, - bool aNotify) - { - return NS_OK; - } + bool aNotify); /** * Hook that is called by Element::SetAttr to allow subclasses to @@ -1412,6 +1408,64 @@ protected: return NS_OK; } + /** + * This function shall be called just before the id attribute changes. It will + * be called after BeforeSetAttr. If the attribute being changed is not the id + * attribute, this function does nothing. Otherwise, it will remove the old id + * from the document's id cache. + * + * This must happen after BeforeSetAttr (rather than during) because the + * the subclasses' calls to BeforeSetAttr may notify on state changes. If they + * incorrectly determine whether the element had an id, the element may not be + * restyled properly. + * + * @param aNamespaceID the namespace of the attr being set + * @param aName the localname of the attribute being set + * @param aValue the new id value. Will be null if the id is being unset. + */ + void PreIdMaybeChange(int32_t aNamespaceID, nsIAtom* aName, + const nsAttrValueOrString* aValue); + + /** + * This function shall be called just after the id attribute changes. It will + * be called before AfterSetAttr. If the attribute being changed is not the id + * attribute, this function does nothing. Otherwise, it will add the new id to + * the document's id cache and properly set the ElementHasID flag. + * + * This must happen before AfterSetAttr (rather than during) because the + * the subclasses' calls to AfterSetAttr may notify on state changes. If they + * incorrectly determine whether the element now has an id, the element may + * not be restyled properly. + * + * @param aNamespaceID the namespace of the attr being set + * @param aName the localname of the attribute being set + * @param aValue the new id value. Will be null if the id is being unset. + */ + void PostIdMaybeChange(int32_t aNamespaceID, nsIAtom* aName, + const nsAttrValue* aValue); + + /** + * Usually, setting an attribute to the value that it already has results in + * no action. However, in some cases, setting an attribute to its current + * value should have the effect of, for example, forcing a reload of + * network data. To address that, this function will be called in this + * situation to allow the handling of such a case. + * + * @param aNamespaceID the namespace of the attr being set + * @param aName the localname of the attribute being set + * @param aValue the value it's being set to represented as either a string or + * a parsed nsAttrValue. + * @param aNotify Whether we plan to notify document observers. + */ + // Note that this is inlined so that when subclasses call it it gets + // inlined. Those calls don't go through a vtable. + virtual nsresult OnAttrSetButNotChanged(int32_t aNamespaceID, nsIAtom* aName, + const nsAttrValueOrString& aValue, + bool aNotify) + { + return NS_OK; + } + /** * Hook to allow subclasses to produce a different EventListenerManager if * needed for attachment of attribute-defined handlers diff --git a/dom/base/nsAttrValueOrString.h b/dom/base/nsAttrValueOrString.h index 8e6c069532..1a62a84287 100644 --- a/dom/base/nsAttrValueOrString.h +++ b/dom/base/nsAttrValueOrString.h @@ -78,6 +78,20 @@ public: return aOther.EqualsAsStrings(*mAttrValue); } + /* + * Returns true if the value stored is empty + */ + bool IsEmpty() const + { + if (mStringPtr) { + return mStringPtr->IsEmpty(); + } + if (mAttrValue) { + return mAttrValue->IsEmptyString(); + } + return true; + } + protected: const nsAttrValue* mAttrValue; mutable const nsAString* mStringPtr; diff --git a/dom/base/nsStyledElement.cpp b/dom/base/nsStyledElement.cpp index e3d8dfe574..8cd448d47e 100644 --- a/dom/base/nsStyledElement.cpp +++ b/dom/base/nsStyledElement.cpp @@ -40,7 +40,6 @@ nsStyledElement::ParseAttribute(int32_t aNamespaceID, nsAttrValue& aResult) { if (aAttribute == nsGkAtoms::style && aNamespaceID == kNameSpaceID_None) { - SetMayHaveStyle(); ParseStyleAttribute(aValue, aResult, false); return true; } @@ -49,6 +48,22 @@ nsStyledElement::ParseAttribute(int32_t aNamespaceID, aResult); } +nsresult +nsStyledElement::BeforeSetAttr(int32_t aNamespaceID, nsIAtom* aName, + const nsAttrValueOrString* aValue, bool aNotify) +{ + if (aNamespaceID == kNameSpaceID_None) { + if (aName == nsGkAtoms::style) { + if (aValue) { + SetMayHaveStyle(); + } + } + } + + return nsStyledElementBase::BeforeSetAttr(aNamespaceID, aName, aValue, + aNotify); +} + nsresult nsStyledElement::SetInlineStyleDeclaration(DeclarationBlock* aDeclaration, const nsAString* aSerialized, diff --git a/dom/base/nsStyledElement.h b/dom/base/nsStyledElement.h index c4894d87f1..cb6dfd54b9 100644 --- a/dom/base/nsStyledElement.h +++ b/dom/base/nsStyledElement.h @@ -80,6 +80,10 @@ protected: * document. */ nsresult ReparseStyleAttribute(bool aForceInDataDoc); + + virtual nsresult BeforeSetAttr(int32_t aNamespaceID, nsIAtom* aName, + const nsAttrValueOrString* aValue, + bool aNotify) override; }; NS_DEFINE_STATIC_IID_ACCESSOR(nsStyledElement, NS_STYLED_ELEMENT_IID) -- cgit v1.2.3