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

RESOLVED FIXED in mozilla29



6 years ago
4 years ago


(Reporter: birtles, Assigned: longsonr)


(Blocks: 1 bug)

Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)



(1 attachment, 1 obsolete attachment)



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

For example:

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

We should ensure these calls are always paired.


6 years ago
QA Contact: general

Comment 1

6 years ago
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

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

Comment 3

4 years ago
Created attachment 8351035 [details] [diff] [review]
Assignee: nobody → longsonr
Attachment #8351035 - Flags: review?(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:

See e.g. AutoAncestorPusher for sample usage:

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

4 years ago
Created attachment 8351380 [details] [diff] [review]
address review comment
Attachment #8351035 - Attachment is obsolete: true
Attachment #8351035 - Flags: review?(dholbert)
Attachment #8351380 - Flags: review?(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
>+  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.

>+  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 */
>+#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

>-  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 on attachment 8351380 [details] [diff] [review]
address review comment

r=me with comment 6 addressed.
Attachment #8351380 - Flags: review?(dholbert) → review+

Comment 8

4 years ago
(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.
(In reply to Robert Longson from comment #10)

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

Comment 12

4 years ago
Oops, I did.
Whiteboard: [leave open]
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.