485107 - Do not throw in NewResolve

Status: RESOLVED FIXED

(Toolkit :: Storage, defect)

Description

[Shawn Wilsher :sdwilsh]

We stop following the prototype chain when we throw.  Bad storage, bad.

Comment 1

[Shawn Wilsher :sdwilsh]

(unless we really really mean to like in http://mxr.mozilla.org/mozilla-central/source/storage/src/mozStorageStatementParams.cpp#202)

Comment 2

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

Comment 3

[Andrew Sutherland [:asuth] (he/him)]

Is it possible to get a unit test for this?  You said we didn't need one on bug 485071#13, but I'm pretty confused about what the current code is breaking.  (I had locally rolled a unit test, and I think it passed...)

Looking at the XPConnect code, I can see how the change allows the dataflow of the object to subsequent processing, but I don't actually understand it, and the documentation on https://developer.mozilla.org/En/NsIXPCScriptable#newResolve.28.29 is not sufficient to enlighten me.

I'm cool if you want to switch the review to :bent or someone to whom it's clear this is the right thing, but I would like a unit test that fails without the change or a pointer to documentation that defines the proper behavior the code is currently failing to meet so that I can be confident without having to read a large portion of the JS/XPC codebase.

Comment 4

[Shawn Wilsher :sdwilsh]

Attachment: v1.1

With a test.  The test failed without this patch, and passes with it.  This depends on my fix in bug 489889

Comment 5

[Shawn Wilsher :sdwilsh]

Comment on attachment 374364 [details] [diff] [review]
v1.1

Trying to free up asuth's review queue just a bit.

Comment 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 374364 [details] [diff] [review]
v1.1

>+    if (NS_FAILED(rv)) {
>+      // It's highly likely that the name doesn't exist, so let the JS engine
>+      // check the prototype chain and throw if that doesn't have the property
>+      // either.
>+      *_objp = aScopeObj;
>+      return NS_OK;
>+    }

According to mrbkap this causes the JS engine to do extra work. Set *_objp to NULL and return NS_OK.

r=me with that.

Comment 7

[Shawn Wilsher :sdwilsh]

(In reply to comment #6)
> According to mrbkap this causes the JS engine to do extra work. Set *_objp to
> NULL and return NS_OK.
Ugh - one of you told me to do that in the first place :/

Will get a new patch up, and a new bug to fix it in a few other places.

Comment 8

[Shawn Wilsher :sdwilsh]

Attachment: v1.2

I had to refactor the tests just a bit to compensate for the assertion added in bug 489889.  Nothing major though, so not getting a new review.  This is good to land.

Comment 9

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/mozilla-central/rev/c30ed3e7e0ce