Last Comment Bug 786801 - document.all does not work in Firefox version 15.0 when prefixed with a frame name
: document.all does not work in Firefox version 15.0 when prefixed with a frame...
Status: RESOLVED FIXED
[js:p1:fx18][js:ni]
: regression
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: 15 Branch
: x86 Windows XP
: -- normal (vote)
: mozilla18
Assigned To: Luke Wagner [:luke]
:
Mentors:
http://matti.no-ip.org/bug786801/runM...
: 792358 (view as bug list)
Depends on: 707492 801325 822171
Blocks: cpg
  Show dependency treegraph
 
Reported: 2012-08-29 13:23 PDT by Jim Tubman
Modified: 2012-12-16 22:26 PST (History)
22 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
affected
+
fixed
+
fixed
+
fixed


Attachments
This is a working example of code that works in every browser except Firefox version 15.0 (775 bytes, application/zip)
2012-08-29 13:23 PDT, Jim Tubman
no flags Details
fix, needs test (4.14 KB, patch)
2012-09-04 12:22 PDT, Luke Wagner [:luke]
no flags Details | Diff | Review
Mochitest for this (1.66 KB, patch)
2012-09-04 14:19 PDT, Boris Zbarsky [:bz] (Out June 25-July 6)
no flags Details | Diff | Review
fix and test (5.60 KB, patch)
2012-09-04 14:30 PDT, Luke Wagner [:luke]
bhackett1024: review+
Details | Diff | Review
fix perf regression (71 bytes, patch)
2012-09-19 15:10 PDT, Luke Wagner [:luke]
no flags Details | Diff | Review
fix perf regression (4.81 KB, patch)
2012-09-19 15:13 PDT, Luke Wagner [:luke]
jorendorff: review+
Details | Diff | Review
combined patch (8.95 KB, patch)
2012-09-20 10:04 PDT, Luke Wagner [:luke]
luke: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Review
mozilla-beta patch (9.62 KB, patch)
2012-09-20 10:08 PDT, Luke Wagner [:luke]
dmandelin: review+
jorendorff: review+
akeybl: approval‑mozilla‑beta+
Details | Diff | Review

Description Jim Tubman 2012-08-29 13:23:35 PDT
Created attachment 656580 [details]
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] 2012-08-29 15:33:33 PDT
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 [:bz] (Out June 25-July 6) 2012-08-29 15:42:42 PDT
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 [:bz] (Out June 25-July 6) 2012-08-29 15:59:14 PDT
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 [:bz] (Out June 25-July 6) 2012-08-29 16:01:23 PDT
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] 2012-08-29 16:16:13 PDT
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 [:bz] (Out June 25-July 6) 2012-08-29 17:42:16 PDT
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] 2012-09-04 09:30:57 PDT
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 [:bz] (Out June 25-July 6) 2012-09-04 09:47:12 PDT
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] 2012-09-04 12:15:42 PDT
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] 2012-09-04 12:18:00 PDT
Guess I'll take it after all :)
Comment 11 Luke Wagner [:luke] 2012-09-04 12:22:47 PDT
Created attachment 658184 [details] [diff] [review]
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 [:bz] (Out June 25-July 6) 2012-09-04 14:19:27 PDT
Created attachment 658229 [details] [diff] [review]
Mochitest for this
Comment 13 Luke Wagner [:luke] 2012-09-04 14:30:26 PDT
Created attachment 658235 [details] [diff] [review]
fix and test

Fix with bz's test.
Comment 15 Luke Wagner [:luke] 2012-09-04 16:54:18 PDT
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] 2012-09-04 19:38:23 PDT
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] 2012-09-05 08:58:51 PDT
That's odd.  The try run was all green...
https://tbpl.mozilla.org/?tree=Try&rev=4b50a37552ce
Comment 18 Luke Wagner [:luke] 2012-09-05 10:35:13 PDT
Ah, that would be because I ran linux64 and osx and the failures were linux32 and windows.  Le sigh.
Comment 19 Luke Wagner [:luke] 2012-09-05 14:30:38 PDT
Ok, I can reproduce the failure on my cset but also the parent cset.  Looking into this more...
Comment 20 Alex Keybl [:akeybl] 2012-09-05 15:52:12 PDT
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] 2012-09-05 17:23:47 PDT
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] 2012-09-12 10:52:05 PDT
What are next steps here?
Comment 23 Jason Orendorff [:jorendorff] 2012-09-13 12:16:47 PDT
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] 2012-09-13 14:41:11 PDT
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] (needinfo me or ping on irc) 2012-09-13 14:48:54 PDT
(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] 2012-09-13 14:59:23 PDT
Thanks, Joe.

New Try run: https://tbpl.mozilla.org/?tree=Try&rev=a21413e88438
Comment 27 Jason Orendorff [:jorendorff] 2012-09-14 04:57:46 PDT
Well, there is orange, but I can't get the logs to load.
Comment 28 Ryan VanderMeulen [:RyanVM] 2012-09-14 05:06:31 PDT
You can load the log from the FTP directory.
Comment 29 alex 2012-09-15 05:23:57 PDT
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] 2012-09-15 05:27:25 PDT
Test builds from comment 26 are available here:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jorendorff@mozilla.com-a21413e88438/
Comment 31 Ryan VanderMeulen [:RyanVM] 2012-09-15 05:29:31 PDT
The Try run is still showing the same failures as comment 16, BTW.
Comment 32 Masatoshi Kimura [:emk] 2012-09-15 05:34:25 PDT
(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 2012-09-15 07:43:32 PDT
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] 2012-09-17 18:28:05 PDT
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] (needinfo me or ping on irc) 2012-09-18 00:33:45 PDT
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] 2012-09-18 09:53:56 PDT
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] 2012-09-18 12:22:22 PDT
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] (needinfo me or ping on irc) 2012-09-19 00:52:21 PDT
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] 2012-09-19 09:26:34 PDT
(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 40 Jan de Mooij [:jandem] 2012-09-19 09:32:23 PDT
*** Bug 792358 has been marked as a duplicate of this bug. ***
Comment 41 Luke Wagner [:luke] 2012-09-19 11:30:54 PDT
Green on try; relanded, on top of bug 707492:
https://hg.mozilla.org/integration/mozilla-inbound/rev/adab1fdcfe0a
Comment 42 nagarwal 2012-09-19 13:13:20 PDT
Thanks Luke. Is this fix going to be in an upcoming 15.x build.
Comment 43 Luke Wagner [:luke] 2012-09-19 14:29:39 PDT
I think the plan is to land for FF 16 which comes out in a month.
Comment 44 Luke Wagner [:luke] 2012-09-19 14:39:53 PDT
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] 2012-09-19 14:40:28 PDT
(This would explain the gcli timeouts.)
Comment 46 Luke Wagner [:luke] 2012-09-19 15:10:08 PDT
Created attachment 662709 [details] [diff] [review]
fix perf regression
Comment 47 Luke Wagner [:luke] 2012-09-19 15:13:15 PDT
Created attachment 662711 [details] [diff] [review]
fix perf regression

qref'd
Comment 49 Luke Wagner [:luke] 2012-09-19 17:09:04 PDT
Initial talos results seem to indicate that the problem is fixed: http://bit.ly/T61ojW
Comment 50 Luke Wagner [:luke] 2012-09-19 17:30:46 PDT
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] (needinfo me or ping on irc) 2012-09-20 01:44:44 PDT
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] (needinfo me or ping on irc) 2012-09-20 01:45:29 PDT
(And when I say 'pushing the gcli patch' that would include turning the gcli tests off too)
Comment 54 Luke Wagner [:luke] 2012-09-20 10:01:30 PDT
(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] 2012-09-20 10:04:16 PDT
Created attachment 663057 [details] [diff] [review]
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] 2012-09-20 10:08:19 PDT
Created attachment 663059 [details] [diff] [review]
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] 2012-09-20 15:45:31 PDT
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 2012-09-21 15:50:11 PDT
We'll need to see an r+ if this patch is good to go before we can approve for Beta on Monday.
Comment 60 Alex Keybl [:akeybl] 2012-09-24 09:33:34 PDT
(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] 2012-09-24 09:43:33 PDT
(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] 2012-09-24 12:41:01 PDT
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 64 nagarwal 2012-09-26 10:47:40 PDT
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] 2012-09-26 11:06:50 PDT
It will be in the next beta release (due out this week I believe).
Comment 66 nagarwal 2012-09-26 11:08:01 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.