1150297 - Move source property to RegExp instance again.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tooru Fujisawa [:arai]]

cloned from bug 1138325 to Core/JavaScript Engine.

Need to backout the change to source property in bug 1120169 to avoid breaking the webpage uses ClojureScript.

Comment 1

[Tooru Fujisawa [:arai]]

Attachment: Move source property to RegExp instance again. r=till

Carrying forward r=till from bug 1138325 comment #22.

Approval Request Comment
[Feature/regressing bug #]: bug 1120169
[User impact if declined]: webpage uses ClojureScript doesn't work (see bug 1138325 and bug 1149604).
[Describe test coverage new/current, TreeHerder]: This is the change in the spec (ES5 to ES6), both cases are tested in jstests.
[Risks and why]: Low. this patch changes RegExp source property's behavior to previous one.
[String/UUID change made/needed]: None

Comment 2

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8587083 [details] [diff] [review]
Move source property to RegExp instance again. r=till

Should be in 38 beta 3.

Comment 3

[Sylvestre Ledru [:Sylvestre]]

What is the plan for 39 and 40?

Comment 4

[Tooru Fujisawa [:arai]]

Thank you for the approval :)

https://hg.mozilla.org/releases/mozilla-beta/rev/f208b7bb88ae

(In reply to Sylvestre Ledru [:sylvestre] from comment #3)
> What is the plan for 39 and 40?

I guess... it depends on the evangelism? according to hallvors' comment in bug 1138325 comment #24.
Fortunately, "Re-com" page in bug 1149604 gets fixed now :)
If there are still many websites which are not yet fixed when 39 and 40 get beta, I'll port this to them.

Comment 5

[Tooru Fujisawa [:arai]]

Updated following documentations:
  https://developer.mozilla.org/en-US/Firefox/Releases/38
  https://developer.mozilla.org/en-US/Firefox/Releases/38/Site_Compatibility
  https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Regexp/source

Comment 6

[Liz Henry (:lizzard) (relman/hg->git project)]

arai, can you take a look at this again?  Do we need to fix this for 39, 40, 41? Thanks.

Comment 7

[Tooru Fujisawa [:arai]]

Posted comment on bug 1138325 comment #38.  Some websites still use old ClosureScript, and at least one site ( https://racehubhq.com/ ) is affected directly by this.  The page cannot be loaded propery.
I'll prepare for porting the patch to mozilla-beta.

or..., maybe we need to port this to aurora and central too?

Comment 8

[Till Schneidereit [:till]]

(In reply to Tooru Fujisawa [:arai] from comment #7)
> Posted comment on bug 1138325 comment #38.  Some websites still use old
> ClosureScript, and at least one site ( https://racehubhq.com/ ) is affected
> directly by this.  The page cannot be loaded propery.
> I'll prepare for porting the patch to mozilla-beta.

Based on the telemetry results you posted, I'm not sure we should do anything, really: these are really small numbers, after all. Perhaps we should do some tech evangelism for the one site we know is affected, though.
> 
> or..., maybe we need to port this to aurora and central too?

At the very least, we shouldn't back it out of non-release (i.e. Aurora and Nightly) channels, I think.

Comment 9

[Liz Henry (:lizzard) (relman/hg->git project)]

If doing nothing is ok, maybe we can start wontfixing or closing this, and untangle whatever may still need to be done in bug 1120169 and bug 1138325.   Let me know if there turns out to be something to uplift.

Comment 10

[Tooru Fujisawa [:arai]]

Okay, I agree with closing this bug :)

Comment 11

[Tooru Fujisawa [:arai]]

Attachment: (mozilla-beta) Move source property to RegExp instance again. r=till

patch for mozilla-beta (39) here, just in case

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

Comment 12

[Till Schneidereit [:till]]

Comment on attachment 8617727 [details] [diff] [review]
(mozilla-beta) Move source property to RegExp instance again. r=till

Approval Request Comment
[Feature/regressing bug #]: bug 1120169
[User impact if declined]: site breakage, see bug 1172790
[Describe test coverage new/current, TreeHerder]: copiously tested by using the same patch on the previous beta
[Risks and why]: none, straight reversal to previous behavior
[String/UUID change made/needed]: n/a

Comment 13

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8617727 [details] [diff] [review]
(mozilla-beta) Move source property to RegExp instance again. r=till

Approved for uplift to beta based on advice from bz in https://bugzilla.mozilla.org/show_bug.cgi?id=1172790#c7 . 
This is the same thing we did in 38; this patch reverts the behavior that breaks older clojurescript and possibly other sites. 
We will probably need to reassess this for 40 in a few weeks.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6fe433fed5fb

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Backed out for causing various test failures.
https://hg.mozilla.org/releases/mozilla-beta/rev/af8d7ef03660

https://treeherder.mozilla.org/logviewer.html#?job_id=397408&repo=mozilla-beta
https://treeherder.mozilla.org/logviewer.html#?job_id=397406&repo=mozilla-beta

Comment 16

[Tooru Fujisawa [:arai]]

oops, it's a collision with bug 1079919 (Xray for RegExp, and generic toString).
I'll try fixing it, without backing the patch out first (might be better to backout all patches in bug 1079919 tho...)

Comment 17

[Tooru Fujisawa [:arai]]

Attachment: (mozilla-beta) Part 2: Support RegExp source property in XRay.

draft patch for fixing the collision with bug 1079919.

It keeps changes introduced by bug 1079919 (made RegExp.prototype.toString a generic function, and RegExp Xrayable), however, there's a trick:
JSXrayTraits::getOwnPropertyFromTargetIfSafe disallows source property exists both in RegExp prototype and RegExp instance, because it's treated as a situation that instance property shadows prototype property.

http://mxr.mozilla.org/mozilla-beta/source/js/xpconnect/wrappers/XrayWrapper.cpp#318
>      // Disallow any property that shadows something on its (Xrayed)
>      // prototype chain.
>      JSAutoCompartment ac2(cx, wrapper);
>      RootedObject proto(cx);
>      bool foundOnProto = false;
>      if (!JS_GetPrototype(cx, wrapper, &proto) ||
>          (proto && !JS_HasPropertyById(cx, proto, id, &foundOnProto)))
>      {
>          return false;
>      }
>      if (foundOnProto)
>          return ReportWrapperDenial(cx, id, WrapperDenialForXray, "value shadows a property on the standard prototype");

iiuc, we can allow it and skip this check only for RegExp#source, since RegExp's source property is non-configurable, always exists both in instance and prototype, and those properties have no relations, so the *shadowing* doesn't introduce any flaw (am I correct?).  How do you think about adding this exception for fixing this issue?

Try is running (it includes part 1, the patch above, not listed there tho):
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=98c5093f3dda
At least it passes the tests which failed in comment #15, locally.

Comment 18

[Bobby Holley (:bholley)]

Comment on attachment 8620815 [details] [diff] [review]
(mozilla-beta) Part 2: Support RegExp source property in XRay.

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

Sorry for the delay here.

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +433,5 @@
>                  return getOwnPropertyFromTargetIfSafe(cx, target, wrapper, id, desc);
>              }
> +            if (id == GetRTIdByIndex(cx, XPCJSRuntime::IDX_SOURCE)) {
> +                JSAutoCompartment ac(cx, target);
> +                return getOwnPropertyFromTargetIfSafe(cx, target, wrapper, id, desc, true);

Given that 'source' should only be a string, I think we should just manually validate the param here, and avoid adding extra complication to the logic of getOwnPropertyFromTargetIfSafe.

Aside from that, this exception sounds reasonable.

Comment 19

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Support RegExp source property in Xray.

Thank you for your feedback :)

Moved the logic to JSXrayTraits::resolveOwnProperty.

Tried all test runs, there are several failures that may be caused by branch mismatch (central vs beta, like e10s, etc), but others seems fine.
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=32c9d182d441

Comment 20

[Bobby Holley (:bholley)]

Comment on attachment 8622795 [details] [diff] [review]
Part 2: Support RegExp source property in Xray.

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

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +441,5 @@
> +
> +                MOZ_ASSERT(getTargetObject(wrapper) == target);
> +                MOZ_ASSERT(js::IsObjectInContextCompartment(target, cx));
> +                MOZ_ASSERT(WrapperFactory::IsXrayWrapper(wrapper));
> +                MOZ_ASSERT(desc.object() == nullptr);

I don't think any of these assertions are interesting enough to justify the space they take in the complicated code here - please remove them.

@@ +443,5 @@
> +                MOZ_ASSERT(js::IsObjectInContextCompartment(target, cx));
> +                MOZ_ASSERT(WrapperFactory::IsXrayWrapper(wrapper));
> +                MOZ_ASSERT(desc.object() == nullptr);
> +
> +                Rooted<JSPropertyDescriptor> tempDesc(cx);

No need for tempDesc - just pass desc directly.

@@ +450,5 @@
> +
> +                MOZ_ASSERT(tempDesc.object());
> +                MOZ_ASSERT(!tempDesc.hasGetterOrSetter());
> +                MOZ_ASSERT(!tempDesc.configurable());
> +                MOZ_ASSERT(tempDesc.value().isString());

These assertions _are_ interesting. Can you make the .isString() one a MOZ_RELEASE_ASSERT?

@@ +453,5 @@
> +                MOZ_ASSERT(!tempDesc.configurable());
> +                MOZ_ASSERT(tempDesc.value().isString());
> +
> +                desc.assign(tempDesc.get());
> +                return true;

In the string case, we still need to wrap - need a JS_WrapPropertyDescriptor here, and rescope the {}.

Comment 21

[Tooru Fujisawa [:arai]]

Attachment: (mozilla-beta) Part 2: Support RegExp source property in Xray.

Thanks again!
Addressed the review comments.  Let me know if I misunderstood about the "rescope".

Comment 22

[Tooru Fujisawa [:arai]]

Attachment: (mozilla-beta) Part 2: Support RegExp source property in Xray.

Just rebased.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/654e69b95ed7
https://hg.mozilla.org/releases/mozilla-beta/rev/718980ad944b

Comment 24

[Liz Henry (:lizzard) (relman/hg->git project)]

See https://bugzilla.mozilla.org/show_bug.cgi?id=1172790#c8 for more details.

Comment 25

[Tooru Fujisawa [:arai]]

Updated following documents:
  https://developer.mozilla.org/en-US/Firefox/Releases/40
  https://developer.mozilla.org/en-US/Firefox/Releases/40/Site_Compatibility
  https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp/source

Comment 26

[Liz Henry (:lizzard) (relman/hg->git project)]

This fix went into 39, but not 40 or 41. 

Arai are you still working on this?

Comment 27

[Tooru Fujisawa [:arai]]

Thank you for reminder.
It seems that there're still not-fixed websites, posted in bug 1138325 comment #44.
I'll post rebased patches after some tests.

Comment 28

[Tooru Fujisawa [:arai]]

Attachment: (mozilla-beta 40, 41) Move source property to RegExp instance again. r=till

Only trivial rebase for bug 1147817.

I don't see any regression in try run.
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=c7030a276624
some failure on e10s, and webplatform.  those also fail without my patch

Approval Request Comment (for Part 1 and 2)
[Feature/regressing bug #]: bug 1120169
[User impact if declined]: site breakage (bug 1138325 and bug 1172790)
[Describe test coverage new/current, TreeHerder]: tested on previous beta
[Risks and why]: low, reversal to previous behavior
[String/UUID change made/needed]: none

Comment 29

[Tooru Fujisawa [:arai]]

Attachment: (mozilla-beta 40, 41) Support RegExp source property in Xray. r=bholley

Comment 30

[Tooru Fujisawa [:arai]]

Comment on attachment 8640961 [details] [diff] [review]
(mozilla-beta 40, 41) Support RegExp source property in Xray. r=bholley

wrong request :/

Comment 31

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8640960 [details] [diff] [review]
(mozilla-beta 40, 41) Move source property to RegExp instance again. r=till

Beta+ for the backout.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/60c25113956e
https://hg.mozilla.org/releases/mozilla-beta/rev/4e36b11687f7

Comment 33

[Tooru Fujisawa [:arai]]

Thank you lmandel and RyanVM! :D

Changed the documentation to say RegExp.prototype.source is non-Release only
  https://developer.mozilla.org/en-US/Firefox/Releases/38
  https://developer.mozilla.org/en-US/Firefox/Releases/38/Site_Compatibility
  https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp/source

still adding to 41 Site Compatibility, to promote updating ClosureScript
  https://developer.mozilla.org/en-US/Firefox/Releases/41/Site_Compatibility

removed from
  https://developer.mozilla.org/en-US/Firefox/Releases/40
  https://developer.mozilla.org/en-US/Firefox/Releases/39/Site_Compatibility
  https://developer.mozilla.org/en-US/Firefox/Releases/40/Site_Compatibility

Feel free to revert those changes if they're not appropriate ;)

Comment 34

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tooru, do we need to change RegExp source property's behavior for 41 to match what is in 40? I am just following up on this bug to see if there is any action/uplift needed for Beta41? Thanks.

Comment 35

[Tooru Fujisawa [:arai]]

Comment on attachment 8640960 [details] [diff] [review]
(mozilla-beta 40, 41) Move source property to RegExp instance again. r=till

posted recent status on bug 1138325 comment #45, I think it's almost safe to ship it at least for those listed websites.  preparing backport just in case.

These patches are also applicable to 41 beta.  Do I need to re-ask approval?

Try is running: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6deb3223dafd

Comment 36

[Tooru Fujisawa [:arai]]

Comment on attachment 8640960 [details] [diff] [review]
(mozilla-beta 40, 41) Move source property to RegExp instance again. r=till

Looks like no regression happens on try run.

(negative) Approval Request Comment (for Part 1 and 2)
[Feature/regressing bug #]: bug 1120169
[User impact if declined]: possible site breakage (like bug 1138325)
[Describe test coverage new/current, TreeHerder]: tested on previous beta
[Risks and why]: low, reversal to previous behavior
[String/UUID change made/needed]: none

As noted in bug 1138325 comment #45, there are still 5 known websites which uses older version of ClojureScript which has compatibility issue with RegExp.prototype.source change, but none of them hit the issue when opening the page (not exhaustive test tho).  Others are already fixed, like bug bug 1172790.
IMO, it's ready to stop backporting and move RegExp.prototype.source change to mozilla-release.  Of course there a few compat issue should happen after we release this, but waiting won't help much if no one know the issue.  Also, this change is sitting several weeks for each beta until we backout, and currently we don't get another compat issue report since June.  So the risk *not* to land this change to current mozilla-beta should be low enough.

If you agree, please *reject* this approval request ;)

or perhaps, is there anything should I do, investigate, or wait before closing this bug? if so, please tell me.

Comment 37

[Tooru Fujisawa [:arai]]

Comment on attachment 8640960 [details] [diff] [review]
(mozilla-beta 40, 41) Move source property to RegExp instance again. r=till

Clearing requests, as we're going to ship RegExp.prototype.source to mozilla-release (bug 1138325 comment #50).

Comment 38

[Florian Scholz (Open Web Docs)]

See comment 33 for doc updates. Thanks, :arai!

Comment 39

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.