786801 - document.all does not work in Firefox version 15.0 when prefixed with a frame name

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jim Tubman]

Attachment: This is a working example of code that works in every browser except Firefox version 15.0

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:15.0) Gecko/20100101 Firefox/15.0
Build ID: 20120824154833

Steps to reproduce:

The bug is with javascript code.  This was working until today (August 29th 2012), when my Firefox automatically updated to version 15.0.



Actual results:

received a javascript error on a page containing an iframe when trying to access iframeName.document.all.someDiv which worked prior to version 15 (and works on every other browser, Safari, Chrome, IE).


Expected results:

should have returned a handle to my div.

Comment 1

[Matthias Versen [:Matti]]

Last good nightly: 2012-05-04
First bad nightly: 2012-05-05

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2db9df42823d&tochan
ge=0a48e6561534

I guess that this is from bug 561664

Comment 2

[Boris Zbarsky [:bzbarsky]]

This is very unlikely to be bug 561664.

What I get is this:

[18:40:04.334] TypeError: myIFrame.document.all is undefined @ file:///Users/bzbarsky/test/runMe.htm:7

My money is on bug 650353 so far, given the regression range.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Yeah, totally.  The problem is that the cross-compartment get doesn't resolve with JSRESOLVE_QUALIFIED which of course makes document.all not exist.  Stack is more or less like so (some frames elided):

#0  nsHTMLDocumentSH::NewResolve (this=0x1267c3ac0, wrapper=0x12652a2e0, cx=0x126799400, obj=0x11c8a6250, id={asBits = 4778175808}, flags=0, objp=0x7fff5fbf6ee8, _retval=0x7fff5fbf6ef3) at nsDOMClassInfo.cpp:9296
...
#3  0x00000001011abb90 in CallResolveOp (cx=0x126799400, obj={<JS::HandleBase<JSObject *>> = {<No data fields>}, ptr = 0x7fff5fbf7170}, id={<JS::HandleBase<jsid>> = {<No data fields>}, ptr = 0x7fff5fbf73d0}, flags=0, objp={<JS::MutableHandleBase<JSObject *>> = {<No data fields>}, ptr = 0x7fff5fbf73c0}, propp={<JS::MutableHandleBase<js::Shape *>> = {<No data fields>}, ptr = 0x7fff5fbf73b8}, recursedp=0x7fff5fbf715b) at jsobj.cpp:4268
#4  0x00000001011a2ea2 in LookupPropertyWithFlagsInline (cx=0x126799400, obj={<JS::HandleBase<JSObject *>> = {<No data fields>}, ptr = 0x7fff5fbf7560}, id={<JS::HandleBase<jsid>> = {<No data fields>}, ptr = 0x7fff5fbf73d0}, flags=65535, objp={<JS::MutableHandleBase<JSObject *>> = {<No data fields>}, ptr = 0x7fff5fbf73c0}, propp={<JS::MutableHandleBase<js::Shape *>> = {<No data fields>}, ptr = 0x7fff5fbf73b8}) at jsobj.cpp:4320
#5  0x00000001011a40d4 in js_GetPropertyHelperInline (cx=0x126799400, obj={<JS::HandleBase<JSObject *>> = {<No data fields>}, ptr = 0x7fff5fbf7560}, receiver={<JS::HandleBase<JSObject *>> = {<No data fields>}, ptr = 0x7fff5fbf7580}, id_={asBits = 4778175808}, getHow=0, vp={<JS::MutableHandleBase<JS::Value>> = {<JS::MutableValueOperations<JS::MutableHandle<JS::Value> >> = {<JS::ValueOperations<JS::MutableHandle<JS::Value> >> = {<No data fields>}, <No data fields>}, <No data fields>}, ptr = 0x7fff5fbf7568}) at jsobj.cpp:4536
#6  0x00000001011a495b in js::baseops::GetProperty (cx=0x126799400, obj={<JS::HandleBase<JSObject *>> = {<No data fields>}, ptr = 0x7fff5fbf7560}, receiver={<JS::HandleBase<JSObject *>> = {<No data fields>}, ptr = 0x7fff5fbf7580}, id={<JS::HandleBase<jsid>> = {<No data fields>}, ptr = 0x7fff5fbf7578}, vp={<JS::MutableHandleBase<JS::Value>> = {<JS::MutableValueOperations<JS::MutableHandle<JS::Value> >> = {<JS::ValueOperations<JS::MutableHandle<JS::Value> >> = {<No data fields>}, <No data fields>}, <No data fields>}, ptr = 0x7fff5fbf7568}) at jsobj.cpp:4631
#7  0x000000010104b6d8 in JSObject::getGeneric (cx=0x126799400, obj={<JS::HandleBase<JSObject *>> = {<No data fields>}, ptr = 0x7fff5fbf7560}, receiver={<JS::HandleBase<JSObject *>> = {<No data fields>}, ptr = 0x7fff5fbf7580}, id={<JS::HandleBase<jsid>> = {<No data fields>}, ptr = 0x7fff5fbf7578}, vp={<JS::MutableHandleBase<JS::Value>> = {<JS::MutableValueOperations<JS::MutableHandle<JS::Value> >> = {<JS::ValueOperations<JS::MutableHandle<JS::Value> >> = {<No data fields>}, <No data fields>}, <No data fields>}, ptr = 0x7fff5fbf7568}) at jsobjinlines.h:173
#8  0x00000001011eda43 in js::DirectProxyHandler::get (this=0x1016f8498, cx=0x126799400, proxy=0x11c8993c0, receiver_=0x11c8a6250, id_={asBits = 4778175808}, vp=0x7fff5fbf9890) at jsproxy.cpp:614
#9  0x00000001012ba51a in js::DirectWrapper::get (this=0x1016f8488, cx=0x126799400, wrapper=0x11c8993c0, receiver=0x11c8a6250, id={asBits = 4778175808}, vp=0x7fff5fbf9890) at jswrapper.cpp:286
#10 0x00000001012bc246 in js::CrossCompartmentWrapper::get (this=0x1016f8488, cx=0x126799400, wrapperArg=0x11c8993c0, receiverArg=0x11c8993c0, idArg={asBits = 4778175808}, vp=0x7fff5fbf9890) at jswrapper.cpp:517

Comment 4

[Boris Zbarsky [:bzbarsky]]

Oh, cx->resolveFlags = 0xffff throughout this exercise.

So the questions are:

1)  How do we get across the RESOLVE_QUALIFIED?
2)  How do we get across the RESOLVE_DETECTING as needed?

Comment 5

[Luke Wagner [:luke]]

It's unfortunate that the failure-mode here is "returned undefined".  What document.all-detecting code patterns do we fail to recognize if we leave out the second disjunct in
  if (flags & JSRESOLVE_DETECTING || !(flags & JSRESOLVE_QUALIFIED))
?

Comment 6

[Boris Zbarsky [:bzbarsky]]

You mean why do we have the JSRESOLVE_QUALIFIED check?  I'm not sure.  jst, brendan, do you guys recall?

The only cases I can think of offhand where it might matter is "with (document)" and bareword "all" in event handler attributes...

Comment 7

[Luke Wagner [:luke]]

I don't think I'm the right assignee for this one; this seems mostly in DOM/wrapper land.

I am still curious to hear from DOM folks if we can just remove the
  !(flags & JSRESOLVE_QUALIFIED)
disjunct in nsHTMLDocumentSH::{DocumentAllHelperGetProperty,NewResolve} since, if that doesn't break something else, it seems like the simplest fix.

Comment 8

[Boris Zbarsky [:bzbarsky]]

That's not enough for a fix, because the JSRESOLVE_DETECTING handling will still be broken...  We need to actually propagate through the right resolve flags.

Over to jseng, since it's JS proxy code that loses those flags.

Comment 9

[Luke Wagner [:luke]]

Ok: it looks like the problem is pretty simple: the js_InferFlags detecting logic uses cx->stack.currentScript() which intentionally stops at compartment boundaries (it was added for TI, iirc).

Comment 10

[Luke Wagner [:luke]]

Guess I'll take it after all :)

Comment 11

[Luke Wagner [:luke]]

Attachment: fix, needs test

Here's the patch; it fixes my local test-case and the one in the bug URL.  I'll need to learn how to write a mochitest now...

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Mochitest for this

Comment 13

[Luke Wagner [:luke]]

Attachment: fix and test

Fix with bz's test.

Comment 14

[Luke Wagner [:luke]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7887ace84c7d

Comment 15

[Luke Wagner [:luke]]

Comment on attachment 658235 [details] [diff] [review]
fix and test

[Approval Request Comment]
Bug caused by (feature/regressing bug #): cpg / TI combo
User impact if declined: broken sites
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): low

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Unfortunately, I had to back this out for debug mochitest-other test failures and leaks.
https://hg.mozilla.org/integration/mozilla-inbound/rev/04b67eeb817f

https://tbpl.mozilla.org/php/getParsedLog.php?id=14960439&tree=Mozilla-Inbound

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/commandline/test/browser_gcli_web.js | Test timed out
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/commandline/test/browser_gcli_web.js | TypeError: this.element is undefined
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_bfcache.js | an unexpected uncaught JS exception reported through window.onerror - TypeError: console is undefined at chrome://mochitests/content/browser/browser/devtools/commandline/test/browser_gcli_web.js:667
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_aboutSyncProgress.js | leaked until shutdown [nsGlobalWindow #44 about:sync-progress]
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 3613632 bytes during test execution
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 2 instances of AsyncStatement with size 48 bytes each (96 bytes total)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 2062 instances of AtomImpl with size 24 bytes each (49488 bytes total)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of BackstagePass with size 24 bytes
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 2 instances of BodyRule with size 16 bytes each (32 bytes total)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of CalculateFrecencyFunction with size 12 bytes

https://tbpl.mozilla.org/php/getParsedLog.php?id=14961639&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=14961938&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=14961788&tree=Mozilla-Inbound

Comment 17

[Luke Wagner [:luke]]

That's odd.  The try run was all green...
https://tbpl.mozilla.org/?tree=Try&rev=4b50a37552ce

Comment 18

[Luke Wagner [:luke]]

Ah, that would be because I ran linux64 and osx and the failures were linux32 and windows.  Le sigh.

Comment 19

[Luke Wagner [:luke]]

Ok, I can reproduce the failure on my cset but also the parent cset.  Looking into this more...

Comment 20

[Alex Keybl [:akeybl]]

Comment on attachment 658235 [details] [diff] [review]
fix and test

Please re-nominate when any lingering issues are worked out.

Comment 21

[Luke Wagner [:luke]]

I'm actually having trouble finding a cset that doesn't trigger the gcli failure and leak on my 32-bit linux box.

Comment 22

[Alex Keybl [:akeybl]]

What are next steps here?

Comment 23

[Jason Orendorff [:jorendorff]]

Stealing. I'll start by re-reviewing Luke's patch with a fine-toothed comb. As Luke indicated in comment 21, I am pretty skeptical that there is anything wrong with it.

Comment 24

[Jason Orendorff [:jorendorff]]

Well, I couldn't find anything wrong with Luke's patch.

The gcli test intermittently fails; see bug 779168. It was reported the most at right around the time this patch initially landed. I spoke to Joe Walker and he thinks the test is just too large (and synchronous) and needs to be broken into several tests. The bug for that is bug 707492.

browser_dbg_bfcache.js is sketchy too; see bug 782877.

So before spending a ton of time on this I am going to submit the same patch to the try server one more time and let's see how it fares.

Comment 25

[Joe Walker [:jwalker]]

(In reply to Luke Wagner [:luke] from comment #21)
> I'm actually having trouble finding a cset that doesn't trigger the gcli
> failure and leak on my 32-bit linux box.

Assuming this isn't just random, if you can work out what the common factor is here, I'd love to know. That intermittent orange is a mystery to me. I've poked it before and decided that it was rare enough and hard enough to be 'prioritized'.

The most obvious factor is the size of the file 6.4k lines - it's a concatenated set of browser tests.

(In reply to Jason Orendorff [:jorendorff] from comment #24)
> I spoke to Joe Walker
> and he thinks the test is just too large (and synchronous) and needs to be
> broken into several tests. The bug for that is bug 707492.

Actually, I've remembered that it's no-longer synchronous. That was one of my stabs at 'solving' this. The only strange factor that I can think of now is it's size.

For what it's worth:

  $ cat browser_gcli_web.js | grep 'document.all' | wc -l
  0

Comment 26

[Jason Orendorff [:jorendorff]]

Thanks, Joe.

New Try run: https://tbpl.mozilla.org/?tree=Try&rev=a21413e88438

Comment 27

[Jason Orendorff [:jorendorff]]

Well, there is orange, but I can't get the logs to load.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

You can load the log from the FTP directory.

Comment 29

[alex]

hello friends,

with ff 15 we've probably encountered another manifestation of this bug on linux and xp. please try my example case at http://resheteva.org.il/~alex/ff15-frames-bug/index.html. 
i know top.DataView should have been window.frames['DataView'] but before it worked either way and now it works neither way.

may i also download a version that implemented your patch to test our case?

best regards,
alex

Comment 30

[Ryan VanderMeulen [:RyanVM]]

Test builds from comment 26 are available here:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-a21413e88438/

Comment 31

[Ryan VanderMeulen [:RyanVM]]

The Try run is still showing the same failures as comment 16, BTW.

Comment 32

[Masatoshi Kimura [:emk]]

(In reply to alex from comment #29)
> with ff 15 we've probably encountered another manifestation of this bug on
> linux and xp. please try my example case at
> http://resheteva.org.il/~alex/ff15-frames-bug/index.html. 
> i know top.DataView should have been window.frames['DataView'] but before it
> worked either way and now it works neither way.
Probably because Firefox 15 supported DataView interface.
https://www.khronos.org/registry/typedarray/specs/latest/#8
Now top.DataView refers this new interface rather than the lower frame. Your sample do not work with Chrome and IE10 (which also support DataView interface).

Comment 33

[alex]

thank you masatoshi and ryan,

indeed, i had the name chosen a decade ago is now the name of a new interface.
changing that' everything works ok again.
thank you very much for your consideration and time. there's no bug for me here :).

alex

Comment 34

[Luke Wagner [:luke]]

jwalker: I looked into the gcli failure a bit more: the problem seems to be that a Menu object is getting destroyed (with this callstack):
  Menu.prototype.destroy
  SelectionTooltipField.prototype.destroy
  Tooltip.prototype.destroy
  FFDisplay.prototype.destroy
  DTT_destroy
  DTT_test (registerCleanupFunctions callback)
and then subsequently having its 'show' method called (with this callstack):
  Menu.prototype.show
  SelectionTooltipField.prototype.setConversion
  Tooltip.prototype.assignmentConentsChanged
  exports.createEvent/event
  Requisition.prototype._setAssignment
  Requisition.prototype._split
  Requisition.prototype.update
  Inputter.prototype.setInput
  check (browser_gcli_web.js)
  exports.testIncrDecr (browser_gcli_web.js

Any idea how this can happen?  I'll try to look more and see how this patch is causing this.  The test harness doesn't use document.all or warnings-as-errors does it?

Comment 35

[Joe Walker [:jwalker]]

GCLI doesn't use document.all, and I'm not sure what you mean by warnings-as-errors, so I guess that's no too.

You might be interested in the patch in bug 707492, which splits up this test. I'll be kind of sad if we see that as a correct fix for a problem that surfaced while changing the JS engine, but jorendorff seemed to think that could help.

Comment 36

[Luke Wagner [:luke]]

Sweet, with the patch in bug 707492 applied, the failure is fixed in my local build!  Let's try again:
  https://tbpl.mozilla.org/?tree=Try&rev=d436b20db352

> I'll be kind of sad if we see that as a correct fix for a problem that surfaced while 
> changing the JS engine, but jorendorff seemed to think that could help.

IIUC, the test is timing out (intermittently before the patch, always after) so a reasonable explanation is that this patch slows down the JS engine (in debug builds).  In particular, I noticed this test throws a lot of errors (e.g., comment 34, which I suspect also occurs on trunk) and issues a lot of warnings due to missing properties.  This patch fixes a bug where we were previously short-circuiting (and hence speeding up) the missing-property warning path.  This also explains the platform-specific and debug-only failures.

Comment 37

[Luke Wagner [:luke]]

Woohoo, looking green on the linuxen.  Joe, do you think you can land bug 707492?  Also, is the gcli test (that gets split up by bug 707492) on aurora/beta?  Ideally the patch in this bug would land on all those branches.

Comment 38

[Joe Walker [:jwalker]]

Bug 707492 is open for review. Feel free to add your pleas to mine.

I'm not sure what you mean by 'the test is timing out'. If you're saying that there is a bug in Mochitest that is exacerbated by large/slow tests and pushed over the edge into full failure by this patch to the JS engine, then I can understand this being part of a temporary solution.

I don't see anything wrong with this test though. It runs just fine 99.999% of the time here and the same suite passes 100% of the time (as far as I can see) in most major browsers. The errors seem to be a symptom of something else that's broken as far as I can see.

Comment 39

[Luke Wagner [:luke]]

(In reply to Joe Walker [:jwalker] from comment #38)
> Bug 707492 is open for review. Feel free to add your pleas to mine.
> 
> I'm not sure what you mean by 'the test is timing out'.

There seems to be a timer that makes a test fail when it takes too long.  I assume that is why I was getting 100% failure (on trunk) on my linux32 debug builds (which I was running on an older machine) in comment 21.

Comment 41

[Luke Wagner [:luke]]

Green on try; relanded, on top of bug 707492:
https://hg.mozilla.org/integration/mozilla-inbound/rev/adab1fdcfe0a

Comment 42

[nagarwal]

Thanks Luke. Is this fix going to be in an upcoming 15.x build.

Comment 43

[Luke Wagner [:luke]]

I think the plan is to land for FF 16 which comes out in a month.

Comment 44

[Luke Wagner [:luke]]

So the patch seems to regress dromaeo CSS drastically (33%).  I'm fairly certain that the reason is that js_InferFlags is unfortunately hot and StackIter creation is slow (it has to iterate over all compartments to ExpandInlineFrames).  StackSpace::currentScript avoids this by just explicitly handling inline frames when regs.inlined, so I'll go back to using that by adding a parameter that allows cross-compartment scripts.

Comment 45

[Luke Wagner [:luke]]

(This would explain the gcli timeouts.)

Comment 46

[Luke Wagner [:luke]]

Attachment: fix perf regression

Comment 47

[Luke Wagner [:luke]]

Attachment: fix perf regression

qref'd

Comment 48

[Luke Wagner [:luke]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cd63fc21ccfd

Comment 49

[Luke Wagner [:luke]]

Initial talos results seem to indicate that the problem is fixed: http://bit.ly/T61ojW

Comment 50

[Luke Wagner [:luke]]

Actually, the combination of both patches seems to be a reliable 3-4% overall speedup: http://bit.ly/S8p6cy, probably because it removes the extra call to currentScript in Detecting.

Comment 51

[Joe Walker [:jwalker]]

Might that mean that we can push these patch to aurora/beta/etc without pushing the gcli patch too? If so, that would be preferable to me.

Comment 52

[Joe Walker [:jwalker]]

(And when I say 'pushing the gcli patch' that would include turning the gcli tests off too)

Comment 53

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/adab1fdcfe0a
https://hg.mozilla.org/mozilla-central/rev/cd63fc21ccfd

Comment 54

[Luke Wagner [:luke]]

(In reply to Joe Walker [:jwalker] from comment #51)
Yes indeed!  I pushed to try for aurora and beta and both have green mochitest-other w/o the gcli patch.

Comment 55

[Luke Wagner [:luke]]

Attachment: combined patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): TI
User impact if declined: break web compat: document.all misbehavior
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): low

(This patch is a simple fold of the previous two; carrying forward r+.)

Comment 56

[Luke Wagner [:luke]]

Attachment: mozilla-beta patch

The beta patch had a small rebasing conflict in js_GetPropertyHelperInline so I guess I should request explicit review.  (I added the NULL-script check since it is present in aurora/trunk.)

Comment 57

[Alex Keybl [:akeybl]]

Comment on attachment 663057 [details] [diff] [review]
combined patch

[Triage Comment]
Approving for Aurora in preparation for Beta consideration.

Comment 58

[Lukas Blakk [:lsblakk] use ?needinfo]

We'll need to see an r+ if this patch is good to go before we can approve for Beta on Monday.

Comment 59

[Luke Wagner [:luke]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3acb1af786e0

Comment 60

[Alex Keybl [:akeybl]]

(In reply to Luke Wagner [:luke] from comment #55)
> Risk to taking this patch (and alternatives if risky): low

Would you mind being more specific here about where a regression may rear its head? We want to weigh the worst case scenario here against the regression in FF15.

Comment 61

[Luke Wagner [:luke]]

(In reply to Alex Keybl [:akeybl] from comment #60)
The risk of crash in the patch seems very low, given the number of eyes and simplicity of the patch.  With the second iteration of the patch, the risk of perf regression also seems very low given the simplicity of the change (and the speedup measured on talos).  Lastly, this does change behavior of document.all (from broken to fixed) which could potentially break, say, a site that expected the broken behavior; this seems unlikely since the breakage is only FF15.

Comment 62

[Alex Keybl [:akeybl]]

Comment on attachment 663059 [details] [diff] [review]
mozilla-beta patch

[Triage Comment]
Thanks Luke - given that risk evaluation let's land before tomorrow's beta 5 go to build. That'll keep this web regression from lingering for another 6 weeks.

Comment 63

[Luke Wagner [:luke]]

https://hg.mozilla.org/releases/mozilla-beta/rev/07ee72f76387

Comment 64

[nagarwal]

Hi Luke, Alex, thank you very much. Is this fix going to be in a beta update to 16 or it's already updated in a 16 beta.

Comment 65

[Ryan VanderMeulen [:RyanVM]]

It will be in the next beta release (due out this week I believe).

Comment 66

[nagarwal]

(In reply to Ryan VanderMeulen from comment #65)
> It will be in the next beta release (due out this week I believe).

Thanks Ryan very much.