775730 - Make sure all calls to AttributeWillChange are paired with a call to AttributeChanged in SVG

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Brian Birtles (:birtles, Feb 26~ parental leave)]

In some places in content/svg we call AttributeWillChange without following it up with a call to AttributeChanged.

For example:
http://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/DOMSVGLength.cpp#132

If the call to SetUserUnitValue fails we won't call DidChangeLengthList despite calling WillChangeLengthList.

We should ensure these calls are always paired.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

I may have changed my mind. AttributeWillChange could be called if the value stays the same.
But I'll need to add some scriptblockers.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Hmm, looks like some code expects the method calls to come in pairs...

Comment 3

[Robert Longson [:longsonr]]

Attachment: ensure.txt

Comment 4

[Daniel Holbert [:dholbert]]

Thanks for fixing this!

So, ideally, these Auto*Notifier helper-classes should use the MOZILLA_GUARD_OBJECT_* macros, to make sure they're instantiated correctly, as described here:  https://developer.mozilla.org/en-US/docs/Using_RAII_classes_in_Mozilla

See e.g. AutoAncestorPusher for sample usage:
 http://mxr.mozilla.org/mozilla-central/source/layout/style/nsRuleProcessorData.h#239

If you'd like, that could happen as a second patch here. Might be simpler to just do it all in the first patch, though.

Comment 5

[Robert Longson [:longsonr]]

Attachment: address review comment

Comment 6

[Daniel Holbert [:dholbert]]

That was fast! Upcoming are my remaining review comments on the old patch [I didn't look at new one yet -- midaired while posting review comments]:
===
Firstly, one high-level observation: This patch looks like it *might* be a good candidate for using templates to unify all of these helper-classes.  It looks like these Auto*Notifier classes are all basically the same, aside from having a different type, and invoking different WillChange* and DidChange* function. (plus slightly-differing arguments to those functions).

Since the arguments to the WillChange*/DidChange* functions differ, that might make this too tricky to generify... If you can think of a way to do it, that would be great, but I'm OK skipping that since it may be clumsier to shoehorn in the generification due to the different function-signatures.

Anyway, now on to specific comments:

>diff --git a/content/svg/content/src/DOMSVGLength.cpp b/content/svg/content/src/DOMSVGLength.cpp
>+class MOZ_STACK_CLASS AutoChangeLengthNotifier
>+{
>+public:
>+  AutoChangeLengthNotifier(DOMSVGLength* aLength)
>+    : mLength(aLength)
>+  {
>+    MOZ_ASSERT(mLength->HasOwner(),
>+               "Expecting list to have an owner for notification");

Probably worth asserting that mLength is non-null here.

>+private:
>+  DOMSVGLength* mLength;

...and probably worth declaring mLength as 'const', since its value is set in the init list and should never change.

(Those two comments similarly apply for the other Auto*Notifier classes)

>+    float uuPerUnit = InternalItem().GetUserUnitsPerUnit(Element(), Axis());
>+    float newValue = aUserUnitValue / uuPerUnit;
>+    if (uuPerUnit > 0 && NS_finite(newValue)) {
>+      AutoChangeLengthNotifier notifier(this);
>+      InternalItem().SetValueAndUnit(newValue, InternalItem().GetUnit());

The "uuPerUnit > 0" check seems like a "did we just divide by 0?" check, which makes me uneasy (since division by 0 is undefined in C++ and hence compilers are allowed to generate code that does anything at all after it happens.)

Since we ignore the division result in the <=0 case anyway, I'd rather we just did the >0 check before the division, like so:

    float uuPerUnit = InternalItem().GetUserUnitsPerUnit(Element(), Axis());
    if (uuPerUnit > 0) {
      float newValue = aUserUnitValue / uuPerUnit;
      if (NS_finite(newValue)) {
        AutoChangeLengthNotifier notifier(this);
        ...

>+++ b/content/svg/content/src/DOMSVGNumber.cpp
>+namespace mozilla {
>+
>+//----------------------------------------------------------------------
>+// Helper class: AutoChangeNumberNotifier
>+// Stack-based helper class to pair calls to WillChangeNumberList and
>+// DidChangeNumberList.
>+class MOZ_STACK_CLASS AutoChangeNumberNotifier
[...]
>+};
>+
>+}

This one is specifically in its own "namespace mozilla" block, which seems a bit odd, since really most of this file is for code that's implicitly in namespace mozilla. We should probably just put the DOMSVGNumber impl into a large "namespace mozilla {" block (like how DOMSVGLength and DOMSVGLengthList have it), to be consistent. Maybe file a followup on that?

Same goes for DOMSVGPathSeg.cpp. (It could probably share the followup.)

>+++ b/content/svg/content/src/SVGTransform.cpp
>@@ -1,21 +1,27 @@
> /* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*-
>  * vim: sw=2 ts=2 et lcs=trail\:.,tab\:>~ :
>  * This Source Code Form is subject to the terms of the Mozilla Public
>  * License, v. 2.0. If a copy of the MPL was not distributed with this
>  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
>+#include "SVGTransform.h"
>+
> #include "mozilla/dom/SVGTransform.h"

That header you're adding looks the same as the existing first header (but without the path).

So, you probably don't need the new #include, right?

>+namespace {
>+  const double radPerDegree = 2.0 * M_PI / 360.0;
>+}
>+

Add prefix "k" for constant, per https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style#Prefixes

>-  nsresult result = Transform().SetSkewX(angle);
>-  if (NS_FAILED(result)) {
>-    rv.Throw(result);
>+  if (!NS_finite(tan(angle * radPerDegree))) {
>+    rv.Throw(NS_ERROR_RANGE_ERR);
>     return;
>   }
>-  NotifyElementDidChange(emptyOrOldValue);
>+
>+  AutoChangeTransformNotifier notifier(this);
>+  Transform().SetSkewX(angle);

This removes the failure-check for Transform().SetSkewX(). So now, if we change the conditions under which that function fails, this code will silently ignore the failure, which is bad.

We should either:
 - Capture the return value in a DebugOnly<nsresult> variable and assert that it's successful.
...OR, if this is the only caller, maybe:
 - Make nsSVGTransform::SetSkewX() infallible, and have it assert in the condition where it currently fails.

Same goes for SetSkewY.

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 8351380 [details] [diff] [review]
address review comment

r=me with comment 6 addressed.

Comment 8

[Robert Longson [:longsonr]]

(In reply to Daniel Holbert [:dholbert] (mostly away until Jan 2) from comment #6)
> 
> > 
> >+#include "SVGTransform.h"
> >+
> > #include "mozilla/dom/SVGTransform.h"
> 
> That header you're adding looks the same as the existing first header (but
> without the path).

There's two files with the same name in different directories. This is required in order to make it compile now, it picks up the local file rather than the DOM one.

> 
> So, you probably don't need the new #include, right?

Actually I do.

I'll make the other changes.

Comment 9

[Robert Longson [:longsonr]]

https://tbpl.mozilla.org/?tree=Try&rev=cb2e4ac1ea8c

Comment 10

[Robert Longson [:longsonr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ad5d46d03d76

Comment 11

[Daniel Holbert [:dholbert]]

(In reply to Robert Longson from comment #10)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/ad5d46d03d76

From a quick skim, I think that might not have had comment 6 addressed... Did you maybe push the wrong patch version?

Comment 12

[Robert Longson [:longsonr]]

Oops, I did.

Comment 13

[Robert Longson [:longsonr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/db720cc7a830

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/ad5d46d03d76
https://hg.mozilla.org/mozilla-central/rev/db720cc7a830