Move source property to RegExp instance again.

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: arai, Assigned: arai)

Tracking

(4 keywords)

38 Branch
dev-doc-complete, leave-open, regression, site-compat
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox38 fixed, firefox39+ fixed, firefox40+ fixed, firefox41+ wontfix, firefox42+ wontfix, firefox43 wontfix, firefox-esr38 fixed)

Details

(Whiteboard: [DocArea=JS])

Attachments

(5 attachments, 3 obsolete attachments)

16.89 KB, patch
arai
: review+
Details | Diff | Splinter Review
17.29 KB, patch
Details | Diff | Splinter Review
9.64 KB, patch
Bobby Holley (parental leave - send mail for anything urgent)
: review+
Details | Diff | Splinter Review
17.24 KB, patch
arai
: review+
Details | Diff | Splinter Review
9.64 KB, patch
arai
: review+
Details | Diff | Splinter Review
(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
Created attachment 8587083 [details] [diff] [review]
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
Assignee: nobody → arai.unmht
Attachment #8587083 - Flags: review+
Attachment #8587083 - Flags: approval-mozilla-beta?
Keywords: dev-doc-needed, site-compat
Whiteboard: [DocArea=JS]
Comment on attachment 8587083 [details] [diff] [review]
Move source property to RegExp instance again. r=till

Should be in 38 beta 3.
Attachment #8587083 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
What is the plan for 39 and 40?
status-firefox38: --- → affected
(Assignee)

Comment 4

3 years ago
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.
status-firefox38: affected → fixed
Keywords: leave-open
(Assignee)

Comment 5

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

Updated

3 years ago
See Also: → bug 1153963
arai, can you take a look at this again?  Do we need to fix this for 39, 40, 41? Thanks.
status-firefox39: --- → affected
status-firefox40: --- → affected
status-firefox41: --- → affected
tracking-firefox39: --- → +
tracking-firefox40: --- → +
tracking-firefox41: --- → +
Flags: needinfo?(arai.unmht)
(Assignee)

Comment 7

3 years ago
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?
Flags: needinfo?(arai.unmht)
(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.
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.
Keywords: regression
(Assignee)

Comment 10

3 years ago
Okay, I agree with closing this bug :)
status-firefox39: affected → wontfix
(Assignee)

Comment 11

3 years ago
Created attachment 8617727 [details] [diff] [review]
(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 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
Attachment #8617727 - Flags: approval-mozilla-beta?
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.
Attachment #8617727 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
status-firefox39: wontfix → affected
https://hg.mozilla.org/releases/mozilla-beta/rev/6fe433fed5fb
status-firefox39: affected → fixed
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
status-firefox39: fixed → affected
status-firefox-esr38: --- → fixed
(Assignee)

Comment 16

3 years ago
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...)
(Assignee)

Comment 17

3 years ago
Created attachment 8620815 [details] [diff] [review]
(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.
Attachment #8620815 - Flags: feedback?(bobbyholley)
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.
Attachment #8620815 - Flags: feedback?(bobbyholley) → feedback+
(Assignee)

Comment 19

3 years ago
Created attachment 8622795 [details] [diff] [review]
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
Attachment #8620815 - Attachment is obsolete: true
Attachment #8622795 - Flags: review?(bobbyholley)
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 {}.
Attachment #8622795 - Flags: review?(bobbyholley) → review-
(Assignee)

Comment 21

3 years ago
Created attachment 8622803 [details] [diff] [review]
(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".
Attachment #8622795 - Attachment is obsolete: true
Attachment #8622803 - Flags: review?(bobbyholley)
(Assignee)

Comment 22

3 years ago
Created attachment 8623755 [details] [diff] [review]
(mozilla-beta) Part 2: Support RegExp source property in Xray.

Just rebased.
Attachment #8622803 - Attachment is obsolete: true
Attachment #8622803 - Flags: review?(bobbyholley)
Attachment #8623755 - Flags: review?(bobbyholley)
Attachment #8623755 - Flags: review?(bobbyholley) → review+
https://hg.mozilla.org/releases/mozilla-beta/rev/654e69b95ed7
https://hg.mozilla.org/releases/mozilla-beta/rev/718980ad944b
status-firefox39: affected → fixed
See https://bugzilla.mozilla.org/show_bug.cgi?id=1172790#c8 for more details.
(Assignee)

Comment 25

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

Updated

2 years ago
Depends on: 1180075
This fix went into 39, but not 40 or 41. 

Arai are you still working on this?
Flags: needinfo?(arai.unmht)
(Assignee)

Comment 27

2 years ago
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.
Flags: needinfo?(arai.unmht)
status-firefox42: --- → affected
tracking-firefox42: --- → +
(Assignee)

Comment 28

2 years ago
Created attachment 8640960 [details] [diff] [review]
(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
Attachment #8640960 - Flags: review+
Attachment #8640960 - Flags: approval-mozilla-beta?
Comment hidden (obsolete)
(Assignee)

Comment 30

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

wrong request :/
Attachment #8640961 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?
Comment on attachment 8640960 [details] [diff] [review]
(mozilla-beta 40, 41) Move source property to RegExp instance again. r=till

Beta+ for the backout.
Attachment #8640960 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8640961 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-beta/rev/60c25113956e
https://hg.mozilla.org/releases/mozilla-beta/rev/4e36b11687f7
status-firefox40: affected → fixed
(Assignee)

Comment 33

2 years ago
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 ;)
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.
Flags: needinfo?(arai.unmht)
(Assignee)

Comment 35

2 years ago
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
Attachment #8640960 - Attachment description: (mozilla-beta 40) Move source property to RegExp instance again. r=till → (mozilla-beta 40, 41) Move source property to RegExp instance again. r=till
Flags: needinfo?(arai.unmht)
(Assignee)

Updated

2 years ago
Attachment #8640961 - Attachment description: (mozilla-beta 40) Support RegExp source property in Xray. r=bholley → (mozilla-beta 40, 41) Support RegExp source property in Xray. r=bholley
(Assignee)

Comment 36

2 years ago
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.
Attachment #8640960 - Flags: approval-mozilla-beta+ → approval-mozilla-beta?
(Assignee)

Updated

2 years ago
Attachment #8640961 - Flags: approval-mozilla-beta+ → approval-mozilla-beta?
(Assignee)

Comment 37

2 years ago
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).
Attachment #8640960 - Flags: approval-mozilla-beta?
(Assignee)

Updated

2 years ago
Attachment #8640961 - Flags: approval-mozilla-beta?
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox41: affected → wontfix
status-firefox42: affected → wontfix
status-firefox43: --- → wontfix
Resolution: --- → FIXED
See comment 33 for doc updates. Thanks, :arai!
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.