910220 - HTMLDocument throws on assignment to named properties even in non-strict mode

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[David Bruant]

Received on dev-mdc (... can't link as the title is empty or something like that?!):

"In Firefox 21 and Firefox 22,  we are not getting any error in our
web-application.

But in Firefox 23, we are getting following error in a java-script :

*"htmldocument doesn't have an indexed property setter”*

This error is coming at following line in one of our javascript file-

        document[this.name] = this


     When we change this line as following

*     document.getElementById(this.name).value = this*,

      then above error doesn't come and behaviour is good as in FF21 and
FF22


Is there any known change in FF23, due to which above issue is observed?"

They fixed the issue, but this may occur in other websites.

Comment 1

[David Bruant]

Reporter email as sent on dev-mdc: [e-mail]

Comment 2

[Boris Zbarsky [:bzbarsky]]

The exception string is bogus.  HTMLDocument has no indexed anything.  It dates back to before we had named property support; we should fix it.

Here is a minimal-ish testcase for the behavior:

data:text/html,<img id="x"><script>document.x = 5</script><script>alert(document.x)</script>

In Firefox 22 the set is silently ignored

In Firefox 23 the set throws because we get to http://dev.w3.org/2006/webapi/WebIDL/#defineownproperty step 2 substep 2 subsubstep 1, creating is false, there is no named property setter, so we Reject.  This is supposed to throw in strict mode and silently do nothing otherwise, but due to bug 910261 we have to just pick one or the other.  We picked the throwing behavior, but we should maybe change that to the other until the JS folks can get their act together.

As for comment 0, the two lines being cited do totally different things in Firefox 22 (specifically the first line does absolutely nothing while the second changes the .value of some element), so I'm slightly amused that replacing one with the other was considered a reasoanble thing to do, and even more so that it made everything work.  Presumably because doing "document[this.name] = this" is pretty much always a no-op if "this" was the right kind of node in the document anyway...

Comment 3

[Boris Zbarsky [:bzbarsky]]

Of course the big problem with not throwing in strict mode is that once we do it we might never be able to reintroduce the throwing behavior, since sites will come to depend on not throwing....

Comment 4

[Jeff Walden [:Waldo]]

This is bug 826587.  I did a bit of work on a patch for this, back in January or so, that I've been rebasing for months now as I go.  It's a nasty bit of huge patchwork, but it's fairly simple as it goes.  I'd really like to get back to this as soon as possible after Intl's fully enabled and the ES6 testing junk is kicked off enough.

Given this is an error-y case already, from native functions capable of anything, stack introspection's cost would not be the end of the world here.  I think we should hack in whatever sort of friend API is needed, to detect whether the set-attempt is from strict mode code (or Object.defineProperty, I guess -- will need even worse hackery for that) or not, and then use that to determine whether to throw.

Comment 5

[bhavana bajaj [:bajaj]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #4)
> This is bug 826587.  I did a bit of work on a patch for this, back in
> January or so, that I've been rebasing for months now as I go.  It's a nasty
> bit of huge patchwork, but it's fairly simple as it goes.  I'd really like
> to get back to this as soon as possible after Intl's fully enabled and the
> ES6 testing junk is kicked off enough.
> 
> Given this is an error-y case already, from native functions capable of
> anything, stack introspection's cost would not be the end of the world here.
> I think we should hack in whatever sort of friend API is needed, to detect
> whether the set-attempt is from strict mode code (or Object.defineProperty,
> I guess -- will need even worse hackery for that) or not, and then use that
> to determine whether to throw.

Can you please understand if we are seeing any webcompat issues already due to this ?Also if you or bz can help with more information in comment #3 as to how a website or a end-user is impacted by "problem with not throwing in strict mode" . Thanks !

Comment 6

[Jeff Walden [:Waldo]]

Not throwing in strict mode could poison the well for us, as far as implementing correct strict-mode behavior in the future goes.  Could affect other browsers, too, but I'm not so sure, there -- their DOM setups are different enough that they might already do this right, or not at all, and I don't know.  I'd be very leery of not throwing in strict mode.

This would affect any site that doesn't use strict mode, and does something it kind of shouldn't.  The expectation is that doing so will silently do nothing.  But we're throwing, right now.  Unexpected throwing tends to break stuff, including the site here.

As far as approaching this goes:

[20:43] <bz> waldo: so this defineProperty thing
[20:43] <Waldo> yes
[20:43] <bz> waldo: what's the sanest thing we can do for branches?  :(
[20:44] <Waldo> bz: the introspect-the-stack-and-hack thing
[20:44] <bz> ok
[20:44] <Waldo> bz: it shouldn't be too bad, we introspect the stack for other semi-similar things already
[20:44] <bz> Do you have the bandwidth to do it?
[20:44] * bz has no idea how to go about introspecting the JS stack safely nowadays
[20:45] <Waldo> bz: I can at least point you at the existing case where we do it, and the adaptation should be fairly simple -- see js::ReportIfUndeclaredVarAssignment in jsobj.cpp, and the call to it in nsDOMClassInfo.cpp
[20:46] <Waldo> bz: happy to review as well
[20:46] <Waldo> bz: my bandwidth is somewhat uncertain about consing up a patch
[20:47] <bz> ok
[20:47] * bz makes note to self
[20:47] <bz> Waldo: what's the existing case where we do it?
[20:48] <Waldo> bz: exactly that case
[20:48] <bz> Oh, I see
[20:48] <bz> So a question
[20:48] <bz> cx->currentScript is safe to call even if I got called from the jit?
[20:49] * bz wonders what in jitcode updates pc
[20:50] <bz> the jsbytecode* pc
[20:50] <Waldo> bz: jscntxtinlines.h looks like it does special work to be JIT-safe
[20:50] <bz> Ah, nice
[20:50] <Waldo> which is not 100% confirmation, but seems legit
[20:50] <bz> ok, thanks
[20:50] * bz will try to write up a patch for this

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Add friend API for determinining whether we're in a strict-mode script.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Use the new API in codegen to only throw when needed.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Always generate a defineProperty hook for DOM proxies.

This one we could not backport to branches to reduce risk.

Comment 10

[Jeff Walden [:Waldo]]

Comment on attachment 796986 [details] [diff] [review]
part 1.  Add friend API for determinining whether we're in a strict-mode script.

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

::: js/src/jsfriendapi.h
@@ +265,5 @@
>  extern JS_FRIEND_API(bool)
>  ReportIfUndeclaredVarAssignment(JSContext *cx, JS::HandleString propname);
>  
> +/*
> + * Returns whether we're in a strict mode script.  The return value

Add something like "(No script executing is interpreted as not being strict mode.)".

Comment 11

[Jeff Walden [:Waldo]]

Comment on attachment 796994 [details] [diff] [review]
part 2.  Use the new API in codegen to only throw when needed.

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

::: dom/bindings/test/test_defineProperty.html
@@ +42,5 @@
> +  ok(!threw, "Should not throw in non-strict mode when setting named property");
> +}
> +setStrict();
> +setNonStrict();
> +</script>

This could also use tests that Object.defineProperty(document, "x", { get: function() {} }), in strict mode code and in non-strict mode code, throws a TypeError -- also for { value: 17 } or similar.  I'm not entirely sure right now that that's actually the case.  If it isn't, we need to do a little bit more tweaking here.  I don't understand the mapping of generated defineProperty code to WebIDL spec algorithm closely enough to propose fully-insightful additional tests here.

Also it should have the identical set of tests to here, exercised for an object that supports indexed properties.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 1 with changes to be more scoped down to property sets

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 2 merged to the JS engine changes

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 3 fixed up to pass tests too

Comment 15

[Boris Zbarsky [:bzbarsky]]

> Also it should have the identical set of tests to here, exercised for an object that
> supports indexed properties.

That would actually fail without part 3.  I'll add that to part 3.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 2 with test refactored some

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 3 with more tests

Comment 18

[bhavana bajaj [:bajaj]]

I am tracking this, and jfyi our next beta goes to build on MOnday morning PT and if this is low risk enough we should try to get it in by then, else this may end up as a wontfix for Fx24 keeping in mind we shipped with this regression in Fx23.

Comment 19

[Boris Zbarsky [:bzbarsky]]

   https://hg.mozilla.org/integration/mozilla-inbound/rev/f6e727237de2
   https://hg.mozilla.org/integration/mozilla-inbound/rev/a3a28c1a96a7
   https://hg.mozilla.org/integration/mozilla-inbound/rev/b24700fc8592

Comment 20

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 1 for beta and Aurora

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 798133 [details] [diff] [review]
Part 1 for beta and Aurora

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 855971 
User impact if declined: Some web pages will not work correctly
Testing completed (on m-c, etc.): Passes manual tests
Risk to taking this patch (and alternatives if risky): Should be low risk: makes
   us throw in fewer cases. 
String or IDL/UUID changes made by this patch:  None

Comment 22

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 2 for Aurora

Comment 23

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 2 for beta

Comment 24

[Boris Zbarsky [:bzbarsky]]

Part 3 is not needed to fix the regression, and is a bit higher risk (makes us throw where now we do not, to follow spec), so going to keep that trunk-only.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f6e727237de2
https://hg.mozilla.org/mozilla-central/rev/a3a28c1a96a7
https://hg.mozilla.org/mozilla-central/rev/b24700fc8592

Comment 26

[bhavana bajaj [:bajaj]]

Comment on attachment 798135 [details] [diff] [review]
Part 2 for beta

Please land by MOnday morning PT to get into next beta.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/24385c90c9ff
https://hg.mozilla.org/releases/mozilla-aurora/rev/314b9508f907

https://hg.mozilla.org/releases/mozilla-beta/rev/9d1f99575772
https://hg.mozilla.org/releases/mozilla-beta/rev/07ab2af3b812