Closed Bug 1043690 Opened 6 years ago Closed 6 years ago

Verizon webmail gives a null 404 error because <input name="action"> shadows HTMLFormElement.action's setter

Categories

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

31 Branch
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla34
Tracking Status
firefox31 + wontfix
firefox32 + fixed
firefox33 + fixed
firefox34 --- fixed
firefox-esr31 --- fixed

People

(Reporter: mjb, Assigned: bzbarsky)

References

Details

(Keywords: regression, site-compat)

Attachments

(12 files, 4 obsolete files)

294.60 KB, image/png
Details
326 bytes, text/html
Details
381 bytes, text/html
Details
275 bytes, text/html
Details
335 bytes, text/html
Details
665 bytes, text/html
Details
512 bytes, text/html
Details
914 bytes, text/html
Details
812 bytes, text/html
Details
829 bytes, text/html
Details
11.51 KB, patch
efaust
: review+
Details | Diff | Splinter Review
13.90 KB, patch
Details | Diff | Splinter Review
Users on the support forum have come across a new "bug" that when using Firefox 31 they were unable to reply/forward/send e-mails and were given a null 404 error when trying to do so.

Is this a Firefox issue or is it strictly on Verizon's side? It would be nice to take the blame off of Firefox.
Related threads (standard troubleshooting steps exhausted):

https://support.mozilla.org/questions/1006704
https://support.mozilla.org/questions/1012186
(In reply to moses from comment #0)
> Users on the support forum have come across a new "bug" that when using
> Firefox 31 they were unable to reply/forward/send e-mails and were given a
> null 404 error when trying to do so.

Is this a Firefox issue or is it
> strictly on Verizon's side? It would be nice to take the blame off of
> Firefox.

To be more accurate, The problem is unable to reply/forward/save draft e-mails. Sending a new email is OK. Problem does not exist on FireFox V30, but does on V31 with webmail.verizon.net. Error does not happen using Internet Explorer.
Component: Untriaged → General
Hi guys,

Adding comments such as "I have this same issue/Hurry up Mozilla" is generally not helpful and sends additional unnecessary e-mails to everyone CC'd and watching the support.mozilla.org component. Instead of adding "Me too's/Hurry up", please "vote" for this bug to be fixed.

Here's some more information about the Bugzilla Etiquette and info on Voting
Bugzilla Etiquette: https://bugzilla.mozilla.org/page.cgi?id=etiquette.html
Voting: https://bugzilla.mozilla.org/page.cgi?id=voting.html
Just to try and help further the troubleshooting, my Windows 7 Pro, Windows 8.1 Pro (both 64 bit) and Ubuntu 14.04 LTS all exhibit this error with V31.
Firefox Site Compatibility team here. This is the biggest site compat issue so far.

https://input.mozilla.org/en-GB/?q=verizon&product=Firefox&version=31.0
https://input.mozilla.org/en-GB/?q=status%3A404&product=Firefox&version=31.0

https://input.mozilla.org/en-GB/dashboard/response/4535575 has a stack trace in Dojo.

Anyone help figure out the details?
Product: Firefox → Core
Version: unspecified → 31 Branch
Clear steps to reproduce:

1)  Click any email in the Verizon inbox.
2)  Click the "Reply" (curly counterclockwise arrow) button above and to the right of the
    mail.
3)  Click the red "Save Draft" button above the mail header.

One-day regression range on m-c nightlies: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=0e91262606a6&tochange=c67a79064fd4
Bisect says:

The first bad revision is:
changeset:   180282:b588b9285415
user:        Jason Orendorff <jorendorff@mozilla.com>
date:        Fri Apr 25 15:07:18 2014 -0500
summary:     Bug 987007, part 2 - Handle assignment to named and indexed setters without using JSRESOLVE_ASSIGNING. r=bz, r=bholley.
[Tracking Requested - why for this release]: Many Verizon customers are complaining about the issue. No workaround available.
Severity: normal → major
Component: General → DOM
Flags: needinfo?(jorendorff)
This is specifically due to this change in HTMLFormElement's getOwnPropertyDescriptor:

-  if (!IsArrayIndex(index) && !(flags & JSRESOLVE_ASSIGNING) && (!isXray || !HasPropertyOnPrototype(cx, proxy, id))) {
+  if (!IsArrayIndex(index) && (!isXray || !HasPropertyOnPrototype(cx, proxy, id))) {

The particular id we get into trouble with is "action".  Since HTMLFormElement has no indexed or named setter, we aren't outputting a "set" method, so we do a genetic BaseProxyHandler::set, which goes via getOwnPropertyDescriptor.  JSRESOLVE_ASSIGNING is set, since this is a set.

Before the fix for bug 987007 we would allow the set to take place, even if the form had a named prop for the "action" id.

After that fix, we don't allow the set to take place, since the named prop shadows the setter on the prototype (doing a named lookup on the form for "action" returns an <input type="hidden">; I assume it has name="action" set).

Then later the site does:

  var _2b8=form.getAttributeNode("action")

(presumably because doing .action was being shadowed by the input!) and gets null where it used to get an Attr object.

The new behavior is what the Web IDL spec says to do.  Still looking into what other UAs do here.
On that testcase, Firefox 30, Chrome, Safari, and IE 11 output:

  [object HTMLInputElement]
  null
  [object HTMLInputElement]
  http://www.mozilla.org

Firefox 31 outputs:

  [object HTMLInputElement]
  null
  [object HTMLInputElement]
  null

Sounds like we need to figure out how to spec (and implement) the old insane behavior...
(In reply to Boris Zbarsky [:bz] from comment #15)
> Firefox 31 outputs:
> 
>   [object HTMLInputElement]
>   null
>   [object HTMLInputElement]
>   null
> 
> Sounds like we need to figure out how to spec (and implement) the old insane
> behavior...

Are you thinking for just HTMLFormElement's named properties, or named properties in general?
On that last testcase, the output is as follows:

IE11 and Chrome:

[object HTMLInputElement]
[object HTMLInputElement]
http://www.mozilla.org

Firefox 30 and 31 and trunk:

[object HTMLInputElement]
[object HTMLInputElement]
undefined

Safari and Firefox from when we used a resolve hook:

[object HTMLInputElement]
[object HTMLInputElement]
[object HTMLInputElement]

The resolve hook (and Safari) behavior clearly violates the HTML spec.

For the other, the IE/Chrome behavior seems to be basically that the proxy has a set hook which ignores the named prop stuff and just does the set normally.  But then a later get will possibly shadow whatever got set by the set via the named getter.

In other words, the IE/Chrome behavior cannot be represented solely via [[GetOwnProperty]].  It requires an explicit [[Set]] override.

That seems to me to be the simplest way forward here, actually.  Cameron, Jason, thoughts?
Flags: needinfo?(cam)
> Are you thinking for just HTMLFormElement's named properties, or named properties in
> general?

I suspect we want this for [OverrideBuiltins] named properties.  For the non-[OverrideBuiltins] ones there is no issue wrt DOM setters: the property can't shadow the proto chain anyway.  Though the shadowing own value prop behavior attachment 8463221 [details] shows wouldn't happen for non-[OverrideBuiltins] props...  It's just the simplest behavior to implement if you override [[Set]], I expect.
So the [[Set]] behaviour would basically be the same as the standard [[Set]] defined in https://people.mozilla.org/~jorendorff/es6-draft.html#sec-ordinary-object-internal-methods-and-internal-slots-set-p-v-receiver but using OrdinaryGetOwnProperty rather than Web IDL's [[GetOwnProperty]], so that we ignore named properties.

This means that Object.defineProperty(formElement, ...) would get ignored as it uses [[GetOwnProperty]] too.

Should we instead just make [[DefineOwnProperty]] do what we want by allowing it to define real properties on the object (forcing them to be non-configurable), regardless of whether there's a matching named property?
Flags: needinfo?(cam)
Attachment #8463238 - Attachment description: test.html → Testcase for Object.defineProperty
Looks like Object.defineProperty actually defines stuff in both IE and Chrome.

To support that, I agree that we'd need to hook [[DefineOwnProperty]].

That said, just hooking [[DefineOwnProperty]] but not [[Set]] wouldn't make setting work, right?  I mean, if [[GetOwnProperty]] returns a non-writable value descriptor (which is the case here), then the default [[Set]] never calls [[DefineOwnProperty]]...
(In reply to Boris Zbarsky [:bz] from comment #24)
> That said, just hooking [[DefineOwnProperty]] but not [[Set]] wouldn't make
> setting work, right?  I mean, if [[GetOwnProperty]] returns a non-writable
> value descriptor (which is the case here), then the default [[Set]] never
> calls [[DefineOwnProperty]]...

Yes, I meant to mention that.  We don't strictly need to have the returned record be [[Writable]]:true, as long as it is [[Configurable]]:true, according to the invariants in https://people.mozilla.org/~jorendorff/es6-draft.html#sec-invariants-of-the-essential-internal-methods.
Scratch that, [[Set]] of course does bypass all of that if notices a non-writable property.
Attachment #8463416 - Attachment is obsolete: true
So it looks like Safari disallows Object.defineOwnProperty when the name is a named prop, so I don't think we need that part for compat.

So really, we just need to hack the [[Set]] somehow.
Just for completeness' sake.
Boris and I hashed this out on IRC. I'm going to implement a JSAPI entry point that basically just does all the steps in [1] from step 3 onward. That way a proxy handler with a custom getOwnPropertyDescriptor() method can easily implement its own set() method that ensures the custom getOwnPropertyDescriptor() method is *not* called in step 2.

Leaving ni?me to remind me to do that work (tomorrow).

[1]: http://people.mozilla.org/~jorendorff/es6-draft.html#sec-ordinary-object-internal-methods-and-internal-slots-set-p-v-receiver
(In reply to Boris Zbarsky [:bz] from comment #31)
> So it looks like Safari disallows Object.defineOwnProperty when the name is
> a named prop, so I don't think we need that part for compat.

Whew. That would have been a major pain to sort out with Xrays.
> Whew. That would have been a major pain to sort out with Xrays.

Yeah, but check out the attached testcases for the insanity that ensues when it's NOT a named prop at the time of the defineProperty call.  Even without Xrays.
Attachment #8464835 - Flags: review?(efaustbmo)
Attachment #8464835 - Flags: feedback?(bzbarsky)
Flags: needinfo?(jorendorff)
Comment on attachment 8464835 [details] [diff] [review]
bug-1043690-SetPropertyIgnoringNamedGetter-v1.patch

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

I can't speak to whether this solves bz's problem, but the code itself looks fine.

r=me with f=bz and at least the JSMSG_READ_ONLY fix.

::: js/src/jsapi-tests/testSetPropertyIgnoringNamedGetter.cpp
@@ +32,5 @@
> +        if (!DirectProxyHandler::getPropertyDescriptor(cx, proxy, id, &desc))
> +            return false;
> +        return SetPropertyIgnoringNamedGetter(cx, this, proxy, receiver, id, &desc, strict, vp);
> +    }
> +    

nit: whitespace

::: js/src/jsproxy.cpp
@@ +178,5 @@
> +    return SetPropertyIgnoringNamedGetter(cx, this, proxy, receiver, id, &desc, strict, vp);
> +}
> +
> +bool
> +js::SetPropertyIgnoringNamedGetter(JSContext *cx, const BaseProxyHandler *handler,

I understand why there's a need to take a handler here, but from a friendapi exposure standpoint, this is ill-advised at best, to my gut. I wish we lived in a world where this, and the insane looking "if it's not a proxy, or if it's a different proxy" checks below weren't necessary.

@@ +182,5 @@
> +js::SetPropertyIgnoringNamedGetter(JSContext *cx, const BaseProxyHandler *handler,
> +                                   HandleObject proxy, HandleObject receiver,
> +                                   HandleId id, MutableHandle<PropertyDescriptor> desc,
> +                                   bool strict, MutableHandleValue vp)
> +{

OK, so none of this is your fault, but if we're committing violence here, let's take a few moments to fix up the remains? Whether to make either of these is up to you, I guess, but I will formally note them here.

@@ +213,4 @@
>      if (desc.object()) {
>          // Check for read-only properties.
>          if (desc.isReadonly())
>              return strict ? Throw(cx, id, JSMSG_CANT_REDEFINE_PROP) : true;

This is the wrong error message. bz fixed the one above, but missed this one recently. Can we make this JSMSG_READ_ONLY?

@@ +232,5 @@
>              if (!desc.hasGetterObject())
>                  desc.setGetter(JS_PropertyStub);
>          }
>          desc.value().set(vp.get());
> +        return handler->defineProperty(cx, receiver, id, desc);

This call inherits the configurability from the prototype object (ie the getPropertyDescriptor call)? That seems strange...

Many moons ago (if pressed, I could dig up the rev again), this used to fall through, but we "fixed" it so that we didn't lose the PropertyOps from the prototype (good). The patch that did it seems to my eye to just be wrong, and it's been wrong ever since.
Comment on attachment 8464835 [details] [diff] [review]
bug-1043690-SetPropertyIgnoringNamedGetter-v1.patch

Seems to do what I want, yes.
Attachment #8464835 - Flags: feedback?(bzbarsky) → feedback+
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Comment on attachment 8464929 [details] [diff] [review]
part 2.  Change the codegen for DOM proxies to ignore named props when looking up property descriptors on [[Set]]

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

Looks good. r=me

::: dom/bindings/DOMJSProxyHandler.cpp
@@ +241,5 @@
> +  }
> +  if (desc.object()) {
> +    MOZ_ASSERT(desc.object() == proxy);
> +  } else {
> +    // Don't just use getPropertyDescriptor, unlike BaseProxyHandler::set,

man, I wish BPH::set did this instead...
Attachment #8464929 - Flags: review?(efaustbmo) → review+
(In reply to Eric Faust [:efaust] from comment #38)
> > +js::SetPropertyIgnoringNamedGetter(JSContext *cx, const BaseProxyHandler *handler,
> 
> I understand why there's a need to take a handler here, but from a friendapi
> exposure standpoint, this is ill-advised at best, to my gut. I wish we lived
> in a world where this, and the insane looking "if it's not a proxy, or if
> it's a different proxy" checks below weren't necessary.

+∞

> @@ +213,4 @@
> >      if (desc.object()) {
> >          // Check for read-only properties.
> >          if (desc.isReadonly())
> >              return strict ? Throw(cx, id, JSMSG_CANT_REDEFINE_PROP) : true;
> 
> This is the wrong error message. bz fixed the one above, but missed this one
> recently. Can we make this JSMSG_READ_ONLY?

Not in this patch, which is intended to land in aurora, beta, and shipping.

Filed followup bug 1046425 to address this, the weirdness below, and the amazing amount of code duplication here.

> @@ +232,5 @@
> >              if (!desc.hasGetterObject())
> >                  desc.setGetter(JS_PropertyStub);
> >          }
> >          desc.value().set(vp.get());
> > +        return handler->defineProperty(cx, receiver, id, desc);
> 
> This call inherits the configurability from the prototype object (ie the
> getPropertyDescriptor call)? That seems strange...

Yes. This is bug 762751.

BaseProxyHandler::set is generally crazy. Ideally it would just do what 9.1.9 does, though who knows what would break if we actually made that change...
(In reply to Jason Orendorff [:jorendorff] from comment #43) 
> Not in this patch, which is intended to land in aurora, beta, and shipping.
> 
> Filed followup bug 1046425 to address this, the weirdness below, and the
> amazing amount of code duplication here.
> 

Ah, OK. I should have realized from the symptoms that this would need to be uplifted agressively, in which case I agree random non-scoped changes are a bad idea.

Thanks for filing the follow-up. Hopefully we can get that cleaned up soon.
Try didn't like the naming of the protected method because it shadowed the public getOwnPropertyDescriptor, so I renamed the new thing to getOwnPropDescriptor with a comment explaining why the name is different.  https://tbpl.mozilla.org/?tree=Try&rev=f16231f2118f
And actually compiling: https://tbpl.mozilla.org/?tree=Try&rev=e58471dcc16c

Jason, let me know once there's a pushable part 1 here?
Flags: needinfo?(jorendorff)
Attached patch Part 2, updated to build and all (obsolete) — Splinter Review
Attachment #8464929 - Attachment is obsolete: true
Try run says there's a test failure in the xpcshell js/xpconnect/tests/unit/test_writeToGlobalPrototype.js test because the assertion Jason put in BaseProxyHandler::set fails.

We seem to be dealing with an xpc::SandboxProxyHandler.  desc.object() and proxy have the same JSClass ("Proxy"), but are not the same object.  Both are proxies.  The handler of proxy is clearly xpc::SandboxProxyHandler.  The handler of desc.object() is js::CrossCompartmentWrapper (around a BackstagePass, in case that makes a difference).

In any case, sounds like that assert is not ok.
Blocks: 1047122
FYI, I have created a Web Compatibility bug to see if we can contact Verizon and get a fix on their side. Bug 1047122
It doesn't we will succeed but it's worth trying.
Latest two support replies in this thread suggest Verizon might already have changed something: https://support.mozilla.org/questions/1006704?page=3#answer-610547
It appears that as of 8:45 EST 31 July 2014 that this bug has been fixed.  I am now able to reply to messages, forward messages, and save a reply to draft.  These are all the past failures that caused me to enter the trouble report.

Whoever was responsible for the fix,  A GIANT THANK YOU!!!!   I really disliked using Internet Explorer.  Firefox is a 10 times better platform!!

I hope all the other Verizon users on FireFox get the word quickly.
Part 1, v2. Added another boolean to fix the assertions.
Attachment #8466282 - Flags: review?(efaustbmo)
Flags: needinfo?(jorendorff)
Comment on attachment 8466282 [details] [diff] [review]
bug-1043690-SetPropertyIgnoringNamedGetter-v2.patch

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

Man, every time we touch this, we have to work around more brokenness. r=me with a bug number for the followup to fix the sandbox handler that makes this bool parameter necessary.

::: js/src/jsapi-tests/testSetPropertyIgnoringNamedGetter.cpp
@@ +33,5 @@
> +            return false;
> +        return SetPropertyIgnoringNamedGetter(cx, this, proxy, receiver, id, &desc,
> +                                              desc.object() == proxy, strict, vp);
> +    }
> +    

nit: whitespace on previous line
Attachment #8466282 - Flags: review?(efaustbmo) → review+
Attachment #8464835 - Attachment is obsolete: true
Attachment #8464835 - Flags: review?(efaustbmo)
Attachment #8465161 - Attachment is obsolete: true
Comment on attachment 8466282 [details] [diff] [review]
bug-1043690-SetPropertyIgnoringNamedGetter-v2.patch

Approval Request Comment
[Feature/regressing bug #]: Bug 987007
[User impact if declined]: Some sites will be broken
[Describe test coverage new/current, TBPL]: There's decent test coverage for this.
[Risks and why]: Risk should be low: this is basically restoring the Firefox 30 
                 behavior.
[String/UUID change made/needed]: None.

Note that I'm not requesting approval-mozilla-release, since Verizon has put a workaround in place for now.
Attachment #8466282 - Flags: approval-mozilla-beta?
Attachment #8466282 - Flags: approval-mozilla-aurora?
Attachment #8466352 - Flags: approval-mozilla-beta?
Attachment #8466352 - Flags: approval-mozilla-aurora?
Comment on attachment 8466282 [details] [diff] [review]
bug-1043690-SetPropertyIgnoringNamedGetter-v2.patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: Serious web compat regression.
User impact if declined: Some sites won't work.
Fix Landed on Version: 34.
Risk to taking this patch (and alternatives if risky): Risk should be low: this
   is basically restoring the Firefox 30 behavior.
String or UUID changes made by this patch: None.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8466282 - Flags: approval-mozilla-esr31?
Attachment #8466352 - Flags: approval-mozilla-esr31?
I'm waiting on this fix to hit m-c for a few days before uplifting. If it lands on m-c today, we may be able to get this in for beta5.
Summary: Verizon webmail gives a null 404 error → Verizon webmail gives a null 404 error because <input name="action"> shadows HTMLFormElement.action's setter
Duplicate of this bug: 1046698
Attachment #8466352 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8466282 [details] [diff] [review]
bug-1043690-SetPropertyIgnoringNamedGetter-v2.patch

Taking them in aurora for coverage.
Attachment #8466282 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Do you need any help from the QA team to manually verify this fix? 

From what I see it does already have automatic test coverage, and the original issue was already verified by Verizon, so the original issue probably cannot be reproduced (unless there are some other mail clients still affected).
> Do you need any help from the QA team to manually verify this fix? 

No, we're all set here.  Thanks!
Attachment #8466282 - Flags: approval-mozilla-esr31?
Attachment #8466282 - Flags: approval-mozilla-esr31+
Attachment #8466282 - Flags: approval-mozilla-beta?
Attachment #8466282 - Flags: approval-mozilla-beta+
Attachment #8466352 - Flags: approval-mozilla-esr31?
Attachment #8466352 - Flags: approval-mozilla-esr31+
Attachment #8466352 - Flags: approval-mozilla-beta?
Attachment #8466352 - Flags: approval-mozilla-beta+
Component: DOM → DOM: Core & HTML
Regressions: 1600275
You need to log in before you can comment on or make changes to this bug.