Closed Bug 1122781 Opened 6 years ago Closed 6 years ago

add assertions to document and enforce style struct computation dependencies

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: dbaron, Assigned: xidorn)

Details

Attachments

(4 files, 6 obsolete files)

1.68 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
6.32 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
7.51 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
3.20 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
The computation of some style structs depends (sometimes conditionally) on the values in another style struct.

When these dependencies are conditional there's a risk of introducing circular dependencies.

It's also useful when modifying code to be able to see what the existing dependencies are so that we avoid introducing bad ones.

Therefore, we should add some methods that both document the existing dependencies and assert when the documented dependencies are violated.


I have some work in progress patches for this:
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/1cde8fc2668d/struct-computation-dependencies
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/1cde8fc2668d/dependency-check-class
that is still far from complete.

(This came up recently in both bug 1098151 and bug 1055672.)
Attached patch add auto check to nsStyleContext (obsolete) — Splinter Review
Maybe something like this?
Attachment #8550686 - Flags: feedback?(dbaron)
Comment on attachment 8550686 [details] [diff] [review]
add auto check to nsStyleContext

I was a little hesitant to put the data on nsStyleContext, because we might want to care about struct dependencies between style contexts.

In particular, there are various ways in which we might go from one style context to another:
 - parent style context -- basically always ok
 - style context of root element -- need to be much more careful, since we might go from the root to the root

It might be nice to have a mechanism that will assert if we use on a non-root style context code that will break when used on the root.

Then again, that's probably a lot harder, and this is probably 95% of what we want, so maybe it's fine to do it this way.

(Does it catch cases where my first patch is missing dependencies?  It certainly should...)
Attachment #8550686 - Flags: feedback?(dbaron) → feedback+
Assignee: nobody → quanxunzhen
Attachment #8550686 - Attachment is obsolete: true
Update dependencies
Attachment #8553517 - Attachment is obsolete: true
Attachment #8553517 - Flags: review?(dbaron)
Attachment #8553703 - Flags: review?(dbaron)
Ping. What do you think about these patches? Do you prefer implementing the dependency check in some other way?
Flags: needinfo?(dbaron)
What in this approach ensures that the declared dependencies are not circular?  (It's not clear to me at first glance what the chunk of code in generate-stylestructlist.py is trying to do.  It could definitely use better comments.)
Flags: needinfo?(dbaron)
(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #12)
> What in this approach ensures that the declared dependencies are not
> circular?  (It's not clear to me at first glance what the chunk of code in
> generate-stylestructlist.py is trying to do.  It could definitely use better
> comments.)

The code in generate-stylestructlist.py trys to do topological sort based on the dependencies. If it fails to do the sort, there must be some circular dependencies.
Add some comments.
Attachment #8553703 - Attachment is obsolete: true
Attachment #8553703 - Flags: review?(dbaron)
Attachment #8558319 - Flags: review?(dbaron)
Comment on attachment 8558319 [details] [diff] [review]
patch 1 - Declare and statically check dependencies of style structs

Please test that introducing a cyclic dependency causes it to give an error, and say in an additional paragraph in the commit message what you tested and what happened.

>+# Check if there is any problem for the style struct dependencies

Check for problems with style struct dependencies

>+resolved_items = []
>+# This whole loop trys to sort the style structs in topological order

trys -> tries

>+# according to the dependencies. A topological order exists iff there
>+# is no cyclic dependencies between the style structs. It resolves one

is no -> are no

>+# struct each loop, and append the resolved one to |resolved_items|.

loop -> iteration

>+for i in range(count):
>+    # This inner loop pick one style struct which does not have

pick -> picks

>+    else:
>+        import sys
>+        print >>sys.stderr, "ERROR: Cannot resolve style struct dependencies"
>+        print >>sys.stderr, "Resolved items:", " ".join(resolved_items)
>+        unsolved_items = [name for _, name, _, _ in STYLE_STRUCTS
>+                          if name not in resolved_items]
>+        print >>sys.stderr, "Unsolved items:", " ".join(unsolved_items)
>+        exit(1)

Maybe this should say something more explicit, like "The following structs have cyclic dependencies"?

r=dbaron with that
Attachment #8558319 - Flags: review?(dbaron) → review+
Comment on attachment 8553518 [details] [diff] [review]
patch 2 - Generate dependencies table of style structs for runtime check

>+const nsStyleContext::DependencyTable nsStyleContext::sDependencyTable;
>+
>+nsStyleContext::DependencyTable::DependencyTable()
>+{
>+#define STYLE_STRUCT(name, checkdata_cb) \
>+  mDependencies[eStyleStruct_##name] =
>+#define STYLE_STRUCT_DEP(dep) NS_STYLE_INHERIT_BIT(dep) |
>+#define STYLE_STRUCT_END() 0;
>+#include "nsStyleStructList.h"
>+#undef STYLE_STRUCT
>+#undef STYLE_STRUCT_DEP
>+#undef STYLE_STRUCT_END
>+}


It would be better to construct this array as static data by:

 (1) asserting that the style structs in nsStyleStructID.h are listed in order by constructing (by including nsStyleStructList.h) a second ID enum without doing Reset and Inherit structs separately, and then including nsStyleStructList.h again to static_assert check them against the primary IDs.  (That could perhaps be a separate patch.)

Then this dependency table could just be a static array (and should probably be declared with an explicit size).

>+#ifdef DEBUG
>+  struct DependencyTable
>+  {
>+    DependencyTable();
>+    bool CheckDependency(nsStyleStructID aDepender,
>+                         nsStyleStructID aDependee) const
>+    {
>+      return !!(mDependencies[aDepender] &
>+                nsCachedStyleData::GetBitForSID(aDependee));
>+    }

Then this could be a static method on nsStyleContext instead of a nested class.  (The table it uses should be private.)

Otherwise this looks good, but I'd probably like to have a quick look at the revised patch, so marking review-.
Attachment #8553518 - Flags: review?(dbaron) → review-
Attachment #8553519 - Flags: review?(dbaron) → review+
Comment on attachment 8553520 [details] [diff] [review]
patch 4 - Add runtime dependency check of style structs

>Bug 1122781 part 4 - Add runtime dependency check of style structs.

This commit message should be clearer that:
 (1) it's checking struct computation against a known set of
     non-cyclic dependencies

 (2) the checks are only done when we get a struct on a context
     during computation of a struct on the same context

>+    AutoCheckDependency(nsStyleContext* aContext, nsStyleStructID aSID)

Maybe call this aSID the aInnerSID, so that there's a clear
distinction.

Also, maybe rename mOuterStruct to mOuterSID, since it's an SID
rather than a struct pointer?

r=dbaron with that
Attachment #8553520 - Flags: review?(dbaron) → review+
Comment on attachment 8553518 [details] [diff] [review]
patch 2 - Generate dependencies table of style structs for runtime check

>+    bool CheckDependency(nsStyleStructID aDepender,
>+                         nsStyleStructID aDependee) const

Also, I think aOuterSID / aInnerSID terminology would probably be clearer here.  (And the normal English terms would be dependent and ???.)
Does it look better?
Attachment #8558319 - Attachment is obsolete: true
Attachment #8560257 - Flags: review?(dbaron)
Comment on attachment 8560257 [details] [diff] [review]
patch 1 - Declare and statically check dependencies of style structs

Sure, although I think it would be even better to explicitly say in the commit message what you tested that leads to such a message.  (You did test, right?)

(It's patch 2 that I wanted to look at a new version of.)
Attachment #8560257 - Flags: review?(dbaron) → review+
(In reply to David Baron [:dbaron] (UTC+11) (needinfo? for questions) from comment #20)
> Comment on attachment 8560257 [details] [diff] [review]
> patch 1 - Declare and statically check dependencies of style structs
> 
> Sure, although I think it would be even better to explicitly say in the
> commit message what you tested that leads to such a message.  (You did test,
> right?)

Yes, I made Visibility depend on Font, then it reports that error.
Attachment #8553518 - Attachment is obsolete: true
Attachment #8560273 - Flags: review?(dbaron)
Comment on attachment 8560273 [details] [diff] [review]
patch 2 - Generate dependencies table of style structs for runtime check

Review of attachment 8560273 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/nsStyleContext.h
@@ +518,5 @@
>                                   // parent context or owned by mRuleNode.
>    uint32_t                mRefCnt;
> +
> +#ifdef DEBUG
> +  bool CheckDependency(nsStyleStructID aOuterSID,

Forgot |static| here. Added locally.
Comment on attachment 8560273 [details] [diff] [review]
patch 2 - Generate dependencies table of style structs for runtime check

>+static enum DebugStyleStruct {

Before this, maybe add a comment like:

  // Check that the style struct IDs are in the same order as they are
  // in nsStyleStructList.h, since when we set up the IDs, we include
  // the inherited and reset structs separately from nsStyleStructList.h

>+const uint32_t nsStyleContext::sDependencyTable[] = {

Probably put the nsStyleStructID_Length size here too?



Also, given that the CheckDependency function returns whether the
dependency is allowed, probably call it DependencyAllowed.  (The
name CheckDependency makes it sound like it has an assertion inside
the function; the name DependencyAllowed makes it sound like a
function that would be used in the condition of an assertion.)


r=dbaron with that, and your correction  in comment 23.
Attachment #8560273 - Flags: review?(dbaron) → review+
(In reply to David Baron [:dbaron] (UTC+11) (needinfo? for questions) from comment #24)
> Comment on attachment 8560273 [details] [diff] [review]
> patch 2 - Generate dependencies table of style structs for runtime check
> 
> >+const uint32_t nsStyleContext::sDependencyTable[] = {
> 
> Probably put the nsStyleStructID_Length size here too?

I think it is not necessary to declare the length anywhere, and I suspect adding the length only provides limited compile time check. For example, if the number of items is less than nsStyleStructID_Length, it can still pass the compile.

Hence I'd like to add static_assert here to check the array length.
Sounds good.  Omitting the length in the .h and the .cpp and checking it with static_assert and NS_ARRAY_LENGTH is even better.
Add the static_assert. Since static_assert cannot check private member outside the class, I add a static function there.
Attachment #8560273 - Attachment is obsolete: true
Attachment #8560305 - Flags: review?(dbaron)
Comment on attachment 8560305 [details] [diff] [review]
patch 2 - Generate dependencies table of style structs for runtime check

>+static enum DebugStyleStruct {

Removing the "static" here is fine.

>+/* static */ void
>+nsStyleContext::CheckDependencyTableLength()
>+{
>+  static_assert(MOZ_ARRAY_LENGTH(nsStyleContext::sDependencyTable)
>+                  == nsStyleStructID_Length,
>+                "Number of items in dependency table doesn't match IDs");
>+}

Could you just put this static_assert in nsStyleContext's constructor rather than having a fake method for it? (#ifdef DEBUG)

r=dbaron with that
Attachment #8560305 - Flags: review?(dbaron) → review+
You need to log in before you can comment on or make changes to this bug.