NullPointerException in my webapp in Firefox 18

RESOLVED FIXED in Firefox 19

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: bani, Assigned: nbp)

Tracking

(Blocks 1 bug, {regression})

18 Branch
mozilla21
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox18 wontfix, firefox19+ verified, firefox20+ fixed, firefox21+ fixed, b2g18+ fixed)

Details

()

Attachments

(2 attachments, 2 obsolete attachments)

User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:19.0) Gecko/20100101 Firefox/19.0
Build ID: 20130109111322

Steps to reproduce:

My legacy web application stopped working in FF 18 (also don't work in 19 beta)
- all versions below 18 were fine
- it works in 18 Safe Mode (tried without any add-ons)
- it works when I'm trying to debug it with Developert Tools or Firebug
- about:config "jit" properties doesn't affect the issue


Actual results:

i'm getting ".. is null" errors in the application when Safe Mode is not enabled. It is consistent, im getting this issue always. 
(This is an internal application so at this point I'm not able to provide steps to reproduce)

Debug message:
XMLException {name: TypeError
message: element is null
description: undefined
fileName: EventAttributeType.js
lineNumber: 223
number: undefined
stack: EventAttributeType.toObject@http://localhost:8888/service/classes:26816
EventAttribute.toObject@http://localhost:8888/service/classes:26567
EventAttribute.toObject@http://localhost:8888/service/classes:26569
EventAttribute.toObject@http://localhost:8888/service/classes:26569
List.toObject@http://localhost:8888/service/classes:17611
EventAttribute.toObject@http://localhost:8888/service/classes:26574
List.toObject@http://localhost:8888/service/classes:17611
EventAttribute.toObject@http://localhost:8888/service/classes:26573
List.toObject@http://localhost:8888/service/classes:17611
EventAttribute.toObject@http://localhost:8888/service/classes:26574
List.toObject@http://localhost:8888/service/classes:17611
EventAttribute.toObject@http://localhost:8888/service/classes:26573
List.toObject@http://localhost:8888/service/classes:17611
EventAttribute.toObject@http://localhost:8888/service/classes:26574
List.toObject@http://localhost:8888/service/classes:17611
EventAttribute.toObject@http://localhost:8888/service/classes:26568
List.toObject@http://localhost:8888/service/classes:17611
EventAttribute.toObject@http://localhost:8888/service/classes:26573
List.toObject@http://localhost:8888/service/classes:17611
EventAttribute.toObject@http://localhost:8888/service/classes:26568
EventAttribute.toObject@http://localhost:8888/service/classes:26569
List.toObject@http://localhost:8888/service/classes:17611
EventAttribute.toObject@http://localhost:8888/service/classes:26574
List.toObject@http://localhost:8888/service/classes:17611
EventAttribute.toObject@http://localhost:8888/service/classes:26568
EventAttribute.toObject@http://localhost:8888/service/classes:26569
EvalHorizontalParam.toObject@http://localhost:8888/service/classes:25832
List.toObject@http://localhost:8888/service/classes:17611
EvaluationParam.toObject@http://localhost:8888/service/classes:26137
Profile.toObject@http://localhost:8888/service/classes:30203
Group.toObject@http://localhost:8888/service/classes:28082
@http://localhost:8888/service/classes:13341
MainPage.prototype.loadGroup@http://localhost:8888/service/classes:22029
MainPage.prototype.init@http://localhost:8888/service/classes:21537
Page.prototype.init0@http://localhost:8888/service/classes:22379
init@http://localhost:8888/service/MainPage?sid=24604021326557356988566612345525819913128905474571&lang=:6
onload@http://localhost:8888/service/MainPage?sid=24604021326557356988566612345525819913128905474571&lang=:1
}
OS: Windows 8 → All
Hardware: x86_64 → All
Severity: normal → critical
The difference between the safemode and the normal mode is:
a) Javascript jit
b) extensions
c) graphic hardware acceleration
d) default window/toolbar settings are used
e) the social API is disabled

I'm surprised that about "- about:config "jit" properties doesn't affect the issue" 
I would expect that disabling "javascript.options.methodjit.content"  followed by a FF restart doesn't help.

You could search a regression range with our semi-automated tool (http://mozilla.github.com/mozregression/) but I think we still need testcase.
Severity: critical → normal
Thanks Matthias.
Yes I suspected this JIT stuff as well but disabling doesn't help, but Safe Mode does, but I can't really think anything else from the list you gave about safe mode..

I've put together a testcase:
https://vt.dteam.hu:444/ff18/tst.html 
from FF18 will alert "exception", but the expected behaviour is alert "no exception"
Thanks for the testcase !

Last good nightly: 2012-10-30
First bad nightly: 2012-10-31

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e19e170d2f6d&tochange=bed18790882f
Assignee: nobody → general
Status: UNCONFIRMED → NEW
Component: Untriaged → JavaScript Engine
Ever confirmed: true
Keywords: regression
Product: Firefox → Core
I'm stupid, really :-(
javascript.options.ion.content to false fixes the issue

I will bisect the exact changeset ...
The first bad revision is:
changeset:   111708:4a2c17905a17
user:        Nicolas B. Pierron <nicolas.b.pierron@mozilla.com>
date:        Mon Oct 29 14:48:45 2012 -0700
summary:     Bug 792631 - Add IC for missing properties. r=dvander
Blocks: 792631
Going through the testcase and replacing toObject by toObj and toString by toStr does not reproduce the test case., which makes me think of Bug 826124.

I am able to reproduce it with both Ion and JM disabled, which makes the previous bad revision unlikely to be the origin of the failure.

Matti: Can you find another revision which is changing behavior with both JIT disabled ?
(In reply to Nicolas B. Pierron [:pierron] [:nbp] from comment #6)

> Matti: Can you find another revision which is changing behavior with both
> JIT disabled ?

Disabling javascript.options.methodjit.content and javascript.options.ion.content I get "no exception" (=correct behavior) on the testcase with build 20130120001806 and FF18 on Windows

What I'm doing wrong ?
(In reply to Nicolas B. Pierron [:pierron] [:nbp] from comment #6)
> Going through the testcase and replacing toObject by toObj and toString by
> toStr does not reproduce the test case., which makes me think of Bug 826124.
> 
> I am able to reproduce it with both Ion and JM disabled, which makes the
> previous bad revision unlikely to be the origin of the failure.
> 
> Matti: Can you find another revision which is changing behavior with both
> JIT disabled ?

Sorry, apparently the test case is not stand-alone and makes request to a web server.
Would it be possible to make a standalone test case ?
The problem is located in following function:

NodeList.prototype.nextNode = function () {
  return this.item(this.currentIndex++);
}

The problem is that NodeList is a proxy, and "this.curentIndex" is property on the wrapped object.  The path which is checking if a property exists is not taking into account the fact that the object might be a proxy and was inlining undefined as a returned value.

I am testing a 2 lines fix which I hope will fix this bug.
Assignee: general → nicolas.b.pierron
Status: NEW → ASSIGNED
This is the patch. I tested it and it change the reported alert from "exception" to "no exception", on the web page reported previously in this bug.
Attachment #705462 - Flags: review?(dvander)
Please target a low risk uplift for 1/29 to make it into FF19b4. We're only a couple of weeks away from release.
Hmm.  Will this prevent the no-property IC only if the "this" object is a proxy, or if there's a proxy anywhere on its proto chain, or in some other cases too?
Comment on attachment 705462 [details] [diff] [review]
Do not add a no-property stub for proxies.

The patch is wrong(*), the problem seems to be related to the usage of the checkObj which does not see the Proxy, or to the fact the the generated stub does not check correctly the Proxy.

(*) I thought it was good because the Typo forbid the generation of the IC stub and made the test case pass.
Attachment #705462 - Attachment is obsolete: true
Attachment #705462 - Flags: review?(dvander)
Attachment #705662 - Flags: review?(kvijayan)
Posted file Mnimal test case.
This test case throw a message printed in the web console.

bz: Do you know if there is an equivalent to NodeList in the shell which might help to reproduce this error?  This would be way more useful than a browser only test case.
Flags: needinfo?(bzbarsky)
Attachment #705662 - Flags: review?(kvijayan) → review+
Sorry, the previous patch was out-dated, after the comment of Waldo.  I fixed it localy, but forgot to update the one taken by bugzilla.  As discussed this also fix a similar bug which I haven't found in the wild yet, AFAIK.

This should be an easy review, sorry for your time.
Attachment #705662 - Attachment is obsolete: true
Attachment #705698 - Flags: review?(kvijayan)
> Do you know if there is an equivalent to NodeList in the shell which might help to
> reproduce this error?

I don't think so.  We should really add one.

That said, this patch is likely to be a noticeable performance problem for browser if I read it correctly (would still like an answer to comment 12).

Could we skip bailing out if the proxy is a listbase with no expandos?  See IsCacheableListBase and the tests performed in generateCallGetter.  That would help mitigate the performance impact on common cases...
Flags: needinfo?(bzbarsky)
Attachment #705698 - Flags: review?(kvijayan) → review+
(In reply to Boris Zbarsky (:bz) from comment #12)
> Hmm.  Will this prevent the no-property IC only if the "this" object is a
> proxy, or if there's a proxy anywhere on its proto chain, or in some other
> cases too?

This patch is only guarding if the object is a proxy, not if any of its prototype are, but I have to say I am a bit confused.  I think it would be better to ensure that there is no Proxy between the object and the holder of the property.
(In reply to Nicolas B. Pierron [:pierron] [:nbp] from comment #18)
> (In reply to Boris Zbarsky (:bz) from comment #12)
> > Hmm.  Will this prevent the no-property IC only if the "this" object is a
> > proxy, or if there's a proxy anywhere on its proto chain, or in some other
> > cases too?
> 
> This patch is only guarding if the object is a proxy, not if any of its
> prototype are, but I have to say I am a bit confused.  I think it would be
> better to ensure that there is no Proxy between the object and the holder of
> the property.

Jandem confirmed that we are already doing so by guarding that all objects between the object and the holder are natives.
I see.  What about the performance issues I mentioned in comment 17?  Again, the real problem here is not that we have a proxy; it's that the proxy has properties you don't know about.  But for ListBase proxies you can in fact find out about the properties to some extent...  I guess the issue is that even for a listbase proxy properties can magically appear without the JIT finding out; it's just that they can't shadow the proto chain.  :(

We really need a better setup for this stuff somehow.  Especially because in the cases I care about for this IC (CSSDeclaration) all the weirdness is restricted to indices; named properties have completely sane behavior.  Would it make any sense to have a way for a proxy handler to guarantee that it will never do weird stuff with non-indices or something?
(In reply to Boris Zbarsky (:bz) from comment #21)
> I see.  What about the performance issues I mentioned in comment 17?  Again,
> the real problem here is not that we have a proxy; it's that the proxy has
> properties you don't know about.  But for ListBase proxies you can in fact
> find out about the properties to some extent...  I guess the issue is that
> even for a listbase proxy properties can magically appear without the JIT
> finding out; it's just that they can't shadow the proto chain.  :(

ListBase proxy *cannot* shadow the proto chain?  Bug 769911#c7 claims the opposite.

The currentIndex property seen in this bug is not on the Proxy object but on the Object wrapped(*) by the ListBase proxy.  This means that any property added to these objects will go to the wrapped object which is not yet checked & guarded by any Ion cache code.

I just have no precise idea what a ListBase proxy is, is there any documentation on these?  In the mean time I assumed that they where identical to proxies, and Waldo mentioned that proxy are shadowing properties of the proto chain. (as well as Bug 769911#c7)

> We really need a better setup for this stuff somehow.  Especially because in
> the cases I care about for this IC (CSSDeclaration) all the weirdness is
> restricted to indices; named properties have completely sane behavior. 
> Would it make any sense to have a way for a proxy handler to guarantee that
> it will never do weird stuff with non-indices or something?

Now that we have an Element object, maybe it would be interesting to use a substitute of it instead of using a Proxy.  Which will remove the use of the "expando"(*).

Apparently, Ion never checked for the "expando"(*) neither in the lookup, nor during the generation of the ReadSlot.  Which cause the previous error.

(*) expando: object wrapped by the proxy? see Bug 769911.

The current patch replace a bad behavior, which is that Ion ignores modifications of the wrapped object of ListBase proxy, by a correct and potentially slow implementation.

> In the worst case, the current patch can be backported, with some slow downs <

bz: What ensure that ListBase proxy are never providing any way to emulate non-indexed properties which are not stored on the expando?

djvj: Why do we bother with the the lookup on checkedObj while the lookup might potentialy return the wrapped object of the ListBase proxy?  In which case, for ListBase proxies, we would have to modify the generateReadSlot function to support ListBase proxy by guarding on the shape of the wrapped object.
https://hg.mozilla.org/mozilla-central/rev/233677e1f043
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
> ListBase proxy *cannot* shadow the proto chain?

For the things it's a weird proxy for (properties that can magically appear and disappear).  It's got an expando holder object in addition to those bits for just normal properties that script set, but the JIT knows all about that and can interrogate it as needed.

> The currentIndex property seen in this bug is not on the Proxy object but on the Object
> wrapped(*) by the ListBase proxy.

Yes.  All properties set by script go on there, because there's no way to store them on the proxy itself.  Again, the JIT knows all about this object and can interrogate it as needed.

> I just have no precise idea what a ListBase proxy is, is there any documentation on
> these?

A "ListBase" (not named very well at this point) proxy is what we use to implement the WebIDL requirements described in <http://dev.w3.org/2006/webapi/WebIDL/#getownproperty> and <http://dev.w3.org/2006/webapi/WebIDL/#defineownproperty>.  We do this with a C++ proxy handler that implements various traps in an attempt to map those two spec concepts to our internal proxy API, which is a ... bad fit all around.   It's a proxy because that's apparently the supported way of doing things like that in SpiderMonkey nowadays.

In any case, if you look at the links above you'll see that if you ignore [OverrideBuiltins] (which we don't have any of yet, though Peter is working on it) the proxies in question cannot shadow the proto chain via their weird behavior.  They can only shadow it via the "expando object", which is not at all an object wrapped by the "proxy" (again, called that only because that's what SpiderMonkey decided to call this sort of thing).  The "expando object" is what we use to implement the "call the default [[DefineOwnProperty]]" (or [[GetOwnProperty]]) bit at the end of the WebIDL steps.  If we could store those properties directly on the "proxy" object, we would in a heartbeat.  ;)

Does that help at all?  I'm happy to try to write up something more concrete if it would be useful... let me know?

> Now that we have an Element object

Oh?  What's that?  If it's something that lets me implement the index bits of the spec linked to above without using a proxy (in the SpiderMonkey sense), I would love that.

> The current patch replace a bad behavior, which is that Ion ignores modifications of
> the wrapped object of ListBase proxy, by a correct and potentially slow implementation.

Right.  I understand that.  I'm just interested in not hitting the slow case if I can avoid it.  ;)

> bz: What ensure that ListBase proxy are never providing any way to emulate non-indexed
> properties which are not stored on the expando?

For ListBase proxies in general, nothing.

For particular ListBase proxies, the lack of a named getter on the relevant interface.

Right now the JIT can't tell the two cases apart, but we should consider fixing that.  I know Peter was already adding some more smarts to tell apart the [OverrideBuiltins] case from the other case; this could work similarly.  Basically, right now "ListBase" lumps together various things with quite different behaviors, whereas for cases like this we want more fine-grained information about what invariants we can expect the object to provide.
And to be clear, I agree that we needed the fix in this bug.  What I'm after is a followup, possibly depending on other things that need doing, to try to avoid taking this slow path unless we really have to.
Today is our last opportunity to get a fix in for this issue for FF19. Please nominate for uplift asap with a risk profile to be considered for beta 4.
Comment on attachment 705698 [details] [diff] [review]
Do not add read property stub for proxies.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 792631 (combined with others)

User impact if declined: Regress JS behavior, as reported on this website.  Some DOM object might not read correctly by IonMonkey when exetended.

Testing completed (on m-c, etc.): comment #23 2013-01-24 18:10:48 PST

Risk to taking this patch (and alternatives if risky): 
This patch disable any cache optimization made on proxies. This might cause a slow down on DOM nodes which are emulating array behaviors.

String or UUID changes made by this patch: N/A
Attachment #705698 - Flags: approval-mozilla-beta?
Attachment #705698 - Flags: approval-mozilla-aurora?
Blocks: 835884
(In reply to Boris Zbarsky (:bz) from comment #25)
> What I'm after is a followup, […]

filed as Bug 835884.
Comment on attachment 705698 [details] [diff] [review]
Do not add read property stub for proxies.

If perf regressions end up outweighing the web breakage, we'll consider backing out. For now though, let's resolve in FF19. Approved.
Attachment #705698 - Flags: approval-mozilla-beta?
Attachment #705698 - Flags: approval-mozilla-beta+
Attachment #705698 - Flags: approval-mozilla-aurora?
Attachment #705698 - Flags: approval-mozilla-aurora+
Comment on attachment 705698 [details] [diff] [review]
Do not add read property stub for proxies.

This patch should apply cleanly on top of aurora and beta.
Attachment #705698 - Flags: checkin?(ryanvm)
Attachment #705698 - Flags: checkin?(ryanvm)
Comment on attachment 705698 [details] [diff] [review]
Do not add read property stub for proxies.

Thanks RyanVM, that makes a lot of sense.
Attachment #705698 - Flags: approval-mozilla-b2g18+
(In reply to Ryan VanderMeulen from comment #31)
> Is this something we'd want for b2g18 as well?

No, but it does not arm to have it as Ion is disabled at compile time.
Verified the issue with the testcase from comment 15 on Firefox 19.0 beta 1
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/20100101 Firefox/19.0
Build ID: 20130109111322

Verified the fix for Firefox 19.0beta 4
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/20100101 Firefox/19.0
Build ID: 20130130080006

The message is not printed anymore in webconsole. Verified on WIndows 7 x64 and Mac OS X 10.8.
You need to log in before you can comment on or make changes to this bug.