1120168 - Assertion failure at jspropertytree.cpp when using JS_SELF_HOSTED_* macros in RegExp.cpp

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[ziyunfei]

Attachment: Test case

I got this failure when I tried to self-host RegExp.prototype.flags:

!parent->inDictionary(), at /spidermonkey/js/src/jspropertytree.cpp:51

Comment 1

[Tooru Fujisawa [:arai]]

The problem is following:

  1. Intl.js uses RegExp literals
  RegExp class is initialilzed before Self-hosting global is initialized.

  2. Intl.js calls RegExp constructor while initialization
  Almost same as 1, and it requires `RegExp` name defined in Self-hosting global.

  3. We're goint to make RegExp.prototype property self-hosted
  Initializing RegExp class requires Self-hosting global to be initialized before it.


I guess possible solution would be one of following:

  A. Lazify initialization of RegExp instances in Intl.js

  B. Do not use RegExp in Self-hosted JS

and I'm trying to implement A.

Comment 2

[Tooru Fujisawa [:arai]]

Attachment: Do not use RegExp literal and do not call RegExp constructor before self-hosting global is initialized in Intl.js.

Draft patch is here.
It works without any errors
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=6100afe979b9
but it would be nice to cache RegExp instances, or at least generated pattern strings (languageTag, etc) to avoid performance regression. There are some limitations now:

1. Cannot set intrinsic variable after initialization

I cannot store RegExp instance into global variable.

https://hg.mozilla.org/mozilla-central/file/2cb22c058add/js/src/vm/GlobalObject.h#l586>     bool setIntrinsicValue(JSContext *cx, PropertyName *name, HandleValue value) {
> #ifdef DEBUG
>         RootedObject self(cx, this);
>         MOZ_ASSERT(cx->runtime()->isSelfHostingGlobal(self));
> #endif

2. Cannot store non flat string in intrinsic variable

I cannot store generated pattern strings into global variable.

https://hg.mozilla.org/mozilla-central/file/2cb22c058add/js/src/vm/SelfHosting.cpp#l1332
>     } else if (selfHostedValue.isString()) {
>         if (!selfHostedValue.toString()->isFlat())
>             MOZ_CRASH();


What would be the best way to cache them?

Comment 3

[Till Schneidereit [:till]]

Hey, just quickly wanted to tell you that it'll be a few days until I get to reviewing this.

Comment 4

[Till Schneidereit [:till]]

Comment on attachment 8562842 [details] [diff] [review]
Do not use RegExp literal and do not call RegExp constructor before self-hosting global is initialized in Intl.js.

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

This looks pretty good already. I agree with you regarding the performance concerns, though, so we should solve those.

What you can do is encapsulating all the Intl regexps in a holder object that you create during self-hosting initialization. On that, you can create properties as in normal code:

var internalIntlRegExps = std_Object_create(null);
function getUnicodeLocaleExtensionSequenceRE() {
    return internalIntlRegExps.unicodeLocaleExtensionSequenceRE || (internalIntlRegExps.unicodeLocaleExtensionSequenceRE = regexp_construct(unicodeLocaleExtensionSequence));
}

(Obviously with proper line-wrapping and all that. Also, I would just inline the unicodeLocaleExtensionSequence string instead of having it in its own var.)

::: js/src/builtin/RegExp.h
@@ +80,5 @@
>  extern bool
>  regexp_test_no_statics(JSContext *cx, unsigned argc, Value *vp);
>  
> +/*
> + * Implementation of RegExp constructor.

While in line with the comments for the two functions above, I'm not too happy with this comment. If I'd encounter it without thinking about self-hosting, I'd be very confused by it.

Perhaps something like "RegExp constructor function, used for the normal RegExp builtin and as a callable that returns a bare, prototype-less RegExp instance in self-hosting."

Comment 5

[Tooru Fujisawa [:arai]]

Thank you for your feedback!

(In reply to Till Schneidereit [:till] from comment #4)
> Comment on attachment 8562842 [details] [diff] [review]
> Do not use RegExp literal and do not call RegExp constructor before
> self-hosting global is initialized in Intl.js.
> 
> Review of attachment 8562842 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks pretty good already. I agree with you regarding the performance
> concerns, though, so we should solve those.
> 
> What you can do is encapsulating all the Intl regexps in a holder object
> that you create during self-hosting initialization. On that, you can create
> properties as in normal code:
> 
> var internalIntlRegExps = std_Object_create(null);
> function getUnicodeLocaleExtensionSequenceRE() {
>     return internalIntlRegExps.unicodeLocaleExtensionSequenceRE ||
> (internalIntlRegExps.unicodeLocaleExtensionSequenceRE =
> regexp_construct(unicodeLocaleExtensionSequence));
> }
> 
> (Obviously with proper line-wrapping and all that. Also, I would just inline
> the unicodeLocaleExtensionSequence string instead of having it in its own
> var.)

Okay, thanks.
I guess it's better to create properties with undefined (or null?) in top-level code (maybe just after std_Object_create), and get/set the property in get*RE functions, to avoid "reference to undefined property" warning, right?
Or perhaps the warning does not matter?

> ::: js/src/builtin/RegExp.h
> @@ +80,5 @@
> >  extern bool
> >  regexp_test_no_statics(JSContext *cx, unsigned argc, Value *vp);
> >  
> > +/*
> > + * Implementation of RegExp constructor.
> 
> While in line with the comments for the two functions above, I'm not too
> happy with this comment. If I'd encounter it without thinking about
> self-hosting, I'd be very confused by it.
> 
> Perhaps something like "RegExp constructor function, used for the normal
> RegExp builtin and as a callable that returns a bare, prototype-less RegExp
> instance in self-hosting."

What does "a bare, prototype-less RegExp instance" mean here?
regexp_construct function returns normal RegExp instance, which has a prototype.

Comment 6

[Till Schneidereit [:till]]

(In reply to Tooru Fujisawa [:arai] from comment #5)
> Okay, thanks.
> I guess it's better to create properties with undefined (or null?) in
> top-level code (maybe just after std_Object_create), and get/set the
> property in get*RE functions, to avoid "reference to undefined property"
> warning, right?
> Or perhaps the warning does not matter?

Blech, I hate that warning. :(

And yes, what you propose is the right way to work around it.

> What does "a bare, prototype-less RegExp instance" mean here?
> regexp_construct function returns normal RegExp instance, which has a
> prototype.

It does? Hmm, probably because regexps are somewhere between objects and primitives :/ Anyway, I'm pretty sure it doesn't matter here because of the way we use the regexps.

It does, however, mean that my comment change request is pretty bogus, so just ignore it.

Comment 7

[Tooru Fujisawa [:arai]]

Attachment: Do not use RegExp literal and do not call RegExp constructor before self-hosting global is initialized in Intl.js.

Thank you again, I updated the patch.

Almost green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=36cec14d0fda
  (one crash in devtools, but I guess it's not related to this patch)

Comment 8

[André Bargull [:anba]]

Does calling regexp_construct work even if `RegExp.multiline` is set? IOW a regexp_construct_no_statics method is not required?

Comment 9

[Till Schneidereit [:till]]

(In reply to André Bargull from comment #8)
> Does calling regexp_construct work even if `RegExp.multiline` is set? IOW a
> regexp_construct_no_statics method is not required?

Excellent question. Arai, can you verify that/if things work with `RegExp.multiline = true`? If not, please adapt things to a regexp_construct_no_statics. If yes, r=me on the patch.

Comment 10

[Tooru Fujisawa [:arai]]

Attachment: Do not use RegExp literal and do not call RegExp constructor before self-hosting global is initialized in Intl.js.

(In reply to Till Schneidereit [:till] from comment #9)
> (In reply to André Bargull from comment #8)
> > Does calling regexp_construct work even if `RegExp.multiline` is set? IOW a
> > regexp_construct_no_statics method is not required?
> 
> Excellent question. Arai, can you verify that/if things work with
> `RegExp.multiline = true`? If not, please adapt things to a
> regexp_construct_no_statics. If yes, r=me on the patch.

Thank you André for pointing that out :)
It doesn't work correctly, and I replaced it with regexp_construct_no_statics.

Green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b741baa0f46e

Comment 11

[Till Schneidereit [:till]]

Comment on attachment 8566920 [details] [diff] [review]
Do not use RegExp literal and do not call RegExp constructor before self-hosting global is initialized in Intl.js.

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

Thanks, this is top-notch work!

::: js/src/builtin/RegExp.cpp
@@ +269,5 @@
>      CallArgs args = CallArgsFromVp(argc, vp);
>      return CallNonGenericMethod<IsRegExp, regexp_compile_impl>(cx, args);
>  }
>  
> +bool

Please make this static again.

Comment 12

[Tooru Fujisawa [:arai]]

Thank you!

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

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d9a929677d0a