1251791 - Ensure Array, Object, etc. pass instanceof checks across BrowserLoader modules

Status: RESOLVED WORKSFORME

(DevTools :: General, defect, P2)

Description

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

James mentioned that he has seen issues in the past with modules loaded in BrowserLoader where array, object, etc. instances would fail instanceof checks when passed between modules.

In particular, things like this from seamless-immutable would not work correctly:

function isMergableObject(target) {
    return target !== null && typeof target === "object" && !(target instanceof Array) && !(target instanceof Date);
}

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Ensure Array, Object pass instanceof across BrowserLoader modules

James, I tried to start investigating this by making a test to prove the problem exists.  However, my test appears to pass without changes.

Does it seem like I am testing the problem correctly?

Comment 2

[James Long (:jlongster)]

Oh, I remembered the problem wrongly, sorry. I thought each module inherited a unique global, but that's not true. Modules do share builtin Array/Object/etc instances.

The problem is that these are not the same as the `window` that the page is in. So if you update your test so that a file included with a <script> tag defines an array `var foo = [1, 2, 3, 4]`, and then inside a required module you do `foo instanceof Array` it will be false.

This means interactions with external scripts are the problem, but theoretically if you required everything it *might* be ok. It still seems like there would be subtle bugs with DOM APIs that are generating objects that don't share the same prototype.

This is less of a big deal than I thought, given that we'll hopefully rewrite everything as modules.

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to James Long (:jlongster) from comment #2)
> The problem is that these are not the same as the `window` that the page is
> in. So if you update your test so that a file included with a <script> tag
> defines an array `var foo = [1, 2, 3, 4]`, and then inside a required module
> you do `foo instanceof Array` it will be false.

Right, you are correct that this difference does exist. 

> This means interactions with external scripts are the problem, but
> theoretically if you required everything it *might* be ok. It still seems
> like there would be subtle bugs with DOM APIs that are generating objects
> that don't share the same prototype.
> 
> This is less of a big deal than I thought, given that we'll hopefully
> rewrite everything as modules.

I agree, I think it's not too much of a concern, since we are most likely writing our own code as modules or importing third party scripts via our loader.

Should I land the attached test anyway?  This would at least ensure that someone doesn't accidentally change things so that modules suddenly *do* see different instances.

Comment 4

[James Long (:jlongster)]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #3)
> 
> I agree, I think it's not too much of a concern, since we are most likely
> writing our own code as modules or importing third party scripts via our
> loader.

Until recently I would have argued that it's a pretty big problem: given that we want to migrate our code into modules, compatibility with current global-scoped code is a pretty big deal. Even if you don't hit this much, when you do hit it, it's mind-bending (I think I wasted almost a whole day trying to figure out what was going on).

However, I get the feeling that we're doing more fresh rewrites of various parts of the code because there are multiple problems with slowly migration stuff, namely XUL and wanting to move to HTML. I know I'm personally starting another debugger refactor of the UI that does less migration and more rewriting, and I hear the console is going to get some similar love.

> 
> Should I land the attached test anyway?  This would at least ensure that
> someone doesn't accidentally change things so that modules suddenly *do* see
> different instances.

I don't think so, whoever does that would have to be very intentional about it and I don't think it's worth testing.

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Yep, agreed it would be a bigger issue if we were planning to pass lots of things in and out of page scripts and modules.  We could make some tweaks if that continues to come up as an issue.

Resolving this for now without the test.