Closed Bug 599940 Opened 10 years ago Closed 9 years ago

Web Console fails to get results of instanceof correctly

Categories

(DevTools :: General, defect)

defect
Not set

Tracking

(blocking2.0 betaN+)

RESOLVED FIXED
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: rcampbell, Assigned: rcampbell)

References

Details

Attachments

(2 files, 3 obsolete files)

1. Open the Web Console on a page
2. In the JS term, enter: [] instanceof Array
3. Hit enter

You would expect to see True returned, but instead see False.

This is likely because we are running the console's through evalInSandbox and don't have full access to the global scope.

Same applies to ({}) instanceof Object and probably several other examples.
Summary: Web Console fails to get results of instanceof correct → Web Console fails to get results of instanceof correctly
You might also look at bug 597872
Depends on: 597872
Blocks: devtools4b8
Assignee: nobody → mihai.sucan
stealing this bug as I have a fix.

Patch also invokes evalInSandbox with JS 1.8.
Assignee: mihai.sucan → rcampbell
Status: NEW → ASSIGNED
Attached patch v1 (obsolete) — Splinter Review
removes with() as the helper functions are already applied to the sandbox. Removed .wrappedJSObject from _window.

needs a test.
Attachment #484329 - Flags: review?(gavin.sharp)
Discussed this on IRC, but just adding here for the benefit of non-Robs: looks like bug 581069 removed JSTerm helper test coverage (http://hg.mozilla.org/mozilla-central/rev/6160fe29dd37#l3.7), so we should probably add that back before landing this.
(In reply to comment #2)
> stealing this bug as I have a fix.
> 
> Patch also invokes evalInSandbox with JS 1.8.

Why the "default" -> "1.8" change? Is that beneficial? I don't think people will expect |var let;| to throw. Interaction with the page may be affected as well...
Depends on: 605565
adding dependency to bug 605565 as I've updated those tests and they provide a convenient place to attach a new testcase.

Natch: the addition of "1.8" was based on feedback from developers. Maybe we should make it an option?
(In reply to comment #6)
> Natch: the addition of "1.8" was based on feedback from developers.

Ah, in that case I am wrong :)

> should make it an option?

The reviewer will probably decide that. |var let;| doesn't throw now in the error console code input, but this is quite different.
ok, now that we have jsterm helper tests again, this patch breaks them. Back to the drawing board!
Attached file v2-WIP (obsolete) —
WIP patch. Currently fails the jshelper tests due to "Console not defined".
Attachment #484329 - Attachment is obsolete: true
Attachment #484329 - Flags: review?(gavin.sharp)
Attached patch v2-WIP (obsolete) — Splinter Review
now with the patch flag!
Attachment #484503 - Attachment is obsolete: true
Depends on: 604430
Attached patch [checked-in] v3Splinter Review
Changed the way we're invoking evalInSandbox. Removed withs, added helper functions directly to the sandbox object.

Using new constructor method for Sandbox.

Added tests for instanceof and checked for return of XrayWrappers.

This should block as it's a much better implementation of the command-line execution.
Attachment #484504 - Attachment is obsolete: true
Attachment #486087 - Flags: review?(dtownsend)
blocking2.0: --- → ?
almost forgot, removed all instances of unwrap() in the helper methods.
blocking2.0: ? → betaN+
I ran this through try last night. 3 spurious warnings, lots of green.
just to add a little more context to the patch that the summary of this bug doesn't relate:

This fixes:

* removes nested with() calls from the command line (with() is bad!).
* removes all instances of XPCNativeObject.unwrap() from the command line.
* adds the helper functions directly to the sandbox object.
* uses the new constructor mechanism for Sandbox objects.

This fixes the instanceof problem mentioned above as well as eliminates some kludgey code from the js command line. I think it's important, makes the code cleaner and more understandable, and is based on feedback from mrbkap about the proper way to use Sandboxes.
Can you generate a diff -w of this, I think it might make it easier to review.
Attached patch v3-wSplinter Review
Now with -w for easier digestion.
Comment on attachment 486087 [details] [diff] [review]
[checked-in] v3

Looks good
Attachment #486087 - Flags: review?(dtownsend) → review+
I guess this can go into b7; I'm not sure I understand the rush, but I also don't believe that there's any significant risk.
it's low-risk and I think people seeing the words "object XrayWrapper" in front of every object they evaluate will be confusing and weird. Plus, I want the console to work correctly and don't think it should wait.
Keywords: checkin-needed
Comment on attachment 486087 [details] [diff] [review]
[checked-in] v3

http://hg.mozilla.org/mozilla-central/rev/bcfefd30c2a6
Attachment #486087 - Attachment description: v3 → [checked-in] v3
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
No longer depends on: 597872
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.