Issue 8710044: SES: Downgrade FIREFOX_15_FREEZE_PROBLEM and partially repair. (Closed)
|
Created:
12 years, 11 months ago by kpreid2 Modified:
12 years, 11 months ago CC:
google-caja-discuss_googlegroups.com Base URL:
http://google-caja.googlecode.com/svn/trunk/ Visibility:
Public. |
DescriptionWe want to be able to run despite FIREFOX_15_FREEZE_PROBLEM, so
* Rename to FREEZE_IS_FRAME_DEPENDENT since the semantics changed.
* Revise the test to more precisely test for the diagnosed problem and
consider deviations to be new symptoms.
* Add a partial repair which causes Object.freeze to fail if it looks
like it would succeed misleadingly.
* Remove conditional disabling of isFrozen assertion in tests.
@r5356
Patch Set 1 #
Total comments: 10
Patch Set 2 : SES: Downgrade FIREFOX_15_FREEZE_PROBLEM and partially repair. #
MessagesTotal messages: 14
Horrible kludges all around, but this is the best thing I could think of. It does not repair isFrozen, but it tries (unsoundly) to prevent any object from becoming falsely frozen. I've left canRepair: false, which is correct because the repair is not sound, but we should execute anyway since the severity is sufficiently low, yes?
Sign in to reply to this message.
https://codereview.appspot.com/8710044/diff/1/src/com/google/caja/ses/repairE... File src/com/google/caja/ses/repairES5.js (right): https://codereview.appspot.com/8710044/diff/1/src/com/google/caja/ses/repairE... src/com/google/caja/ses/repairES5.js:1855: var frozenInWrongFrame = other.Object(); At this point, nothing is wrong yet, so this seems a confusing name. https://codereview.appspot.com/8710044/diff/1/src/com/google/caja/ses/repairE... src/com/google/caja/ses/repairES5.js:1856: var fwfok; What does fwfok stand for? https://codereview.appspot.com/8710044/diff/1/src/com/google/caja/ses/repairE... src/com/google/caja/ses/repairES5.js:2819: fix('freeze'); What about fix('seal') ?
Sign in to reply to this message.
https://codereview.appspot.com/8710044/diff/1/src/com/google/caja/ses/repairE... File src/com/google/caja/ses/repairES5.js (right): https://codereview.appspot.com/8710044/diff/1/src/com/google/caja/ses/repairE... src/com/google/caja/ses/repairES5.js:1856: var fwfok; On 2013/04/12 19:11:43, MarkM wrote: > What does fwfok stand for? Frozen Wrong Frame Okay? ;)
Sign in to reply to this message.
Thanks for the quick fix! lgtm++ https://codereview.appspot.com/8710044/diff/1/src/com/google/caja/ses/repairE... File src/com/google/caja/ses/repairES5.js (right): https://codereview.appspot.com/8710044/diff/1/src/com/google/caja/ses/repairE... src/com/google/caja/ses/repairES5.js:1870: // adequate repair What condition would fall through to here? https://codereview.appspot.com/8710044/diff/1/src/com/google/caja/ses/repairE... src/com/google/caja/ses/repairES5.js:2826: value: function frameCheckWrapper(obj) { Looks good so long as there's not a significant performance penalty (said Ihab gingerly, given recent events...). :)
Sign in to reply to this message.
FYI this snapshot doesn't pass tests.
Sign in to reply to this message.
On 2013/04/12 20:16:23, kpreid2 wrote: > FYI this snapshot doesn't pass tests. Oh, the failures are all the "weak map key" problem, now failing because Firefox updated to 20.0. Guess I'll have to workaround that before I can finalize this. Not all of the failures seem to involve event objects (e.g. testCanvasContextMethods fails).
Sign in to reply to this message.
switch to firefox 21 (beta), that gets past the weak map key problem On Fri, Apr 12, 2013 at 1:23 PM, <kpreid.switchb.org@gmail.com> wrote: > On 2013/04/12 20:16:23, kpreid2 wrote: >> >> FYI this snapshot doesn't pass tests. > > > Oh, the failures are all the "weak map key" problem, now failing because > Firefox updated to 20.0. Guess I'll have to workaround that before I can > finalize this. > > Not all of the failures seem to involve event objects (e.g. > testCanvasContextMethods fails). > > > > https://codereview.appspot.com/8710044/ > > -- > > ---You received this message because you are subscribed to the Google Groups > "Google Caja Discuss" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to google-caja-discuss+unsubscribe@googlegroups.com. > For more options, visit https://groups.google.com/groups/opt_out. > >
Sign in to reply to this message.
On 2013/04/12 22:06:44, felix8a wrote: > switch to firefox 21 (beta), that gets past the weak map key problem I still get "TypeError: cannot use the given object as a weak map key" with Firefox beta 21.0 on test-domado-foreign (among others). Unless I hear otherwise, I have a plan for a simple workaround for the WeakMap problem (namely, use emulated WeakMap as a fallback for objects which cannot be stored in real WeakMap) (which should also work for the WeakMap in ES5/3 failures), and then get back to this change.
Sign in to reply to this message.
ah, my mistake. the test run I thought was against ff21 was actually against ff19.0.2 On Fri, Apr 12, 2013 at 3:21 PM, <kpreid.switchb.org@gmail.com> wrote: > On 2013/04/12 22:06:44, felix8a wrote: >> >> switch to firefox 21 (beta), that gets past the weak map key problem > > > I still get "TypeError: cannot use the given object as a weak map key" > with Firefox beta 21.0 on test-domado-foreign (among others). > > Unless I hear otherwise, I have a plan for a simple workaround for the > WeakMap problem (namely, use emulated WeakMap as a fallback for objects > which cannot be stored in real WeakMap) (which should also work for the > WeakMap in ES5/3 failures), and then get back to this change. > > https://codereview.appspot.com/8710044/
Sign in to reply to this message.
On Fri, Apr 12, 2013 at 3:21 PM, <kpreid.switchb.org@gmail.com> wrote: > On 2013/04/12 22:06:44, felix8a wrote: > >> switch to firefox 21 (beta), that gets past the weak map key problem >> > > I still get "TypeError: cannot use the given object as a weak map key" > with Firefox beta 21.0 on test-domado-foreign (among others). > > Unless I hear otherwise, I have a plan for a simple workaround for the > WeakMap problem (namely, use emulated WeakMap as a fallback for objects > which cannot be stored in real WeakMap) (which should also work for the > WeakMap in ES5/3 failures), and then get back to this change. That sounds better than a workaround. That sounds like a proper repair for this situation. > > > https://codereview.appspot.**com/8710044/<https://codereview.appspot.com/8710... > > -- > > ---You received this message because you are subscribed to the Google > Groups "Google Caja Discuss" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to google-caja-discuss+**unsubscribe@googlegroups.com<google-caja-discuss%2Bunsu... > . > For more options, visit https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o... > . > > > -- Cheers, --MarkM
Sign in to reply to this message.
We want to be able to run despite FIREFOX_15_FREEZE_PROBLEM, so * Rename to FREEZE_IS_FRAME_DEPENDENT since the semantics changed. * Revise the test to more precisely test for the diagnosed problem and consider deviations to be new symptoms. * Add a partial repair which causes Object.freeze to fail if it looks like it would succeed misleadingly. * Remove conditional disabling of isFrozen assertion in tests.
Sign in to reply to this message.
New snapshot (Patch Set 2). Forgot to respond to MarkM's comments earlier, and have now made changes. https://codereview.appspot.com/8710044/diff/1/src/com/google/caja/ses/repairE... File src/com/google/caja/ses/repairES5.js (right): https://codereview.appspot.com/8710044/diff/1/src/com/google/caja/ses/repairE... src/com/google/caja/ses/repairES5.js:1855: var frozenInWrongFrame = other.Object(); On 2013/04/12 19:11:43, MarkM wrote: > At this point, nothing is wrong yet, so this seems a confusing name. Renamed to frozenInOtherFrame. https://codereview.appspot.com/8710044/diff/1/src/com/google/caja/ses/repairE... src/com/google/caja/ses/repairES5.js:1856: var fwfok; On 2013/04/12 19:11:43, MarkM wrote: > What does fwfok stand for? Renamed to freezeSucceeded. https://codereview.appspot.com/8710044/diff/1/src/com/google/caja/ses/repairE... src/com/google/caja/ses/repairES5.js:1870: // adequate repair On 2013/04/12 19:25:02, ihab.awad wrote: > What condition would fall through to here? This is the state which the repair achieves: the other frame's freeze will throw on foreign objects, so the object never gets a bogus frozen flag (both !isFrozen) but freeze threw when it shouldn't (!fwfok). https://codereview.appspot.com/8710044/diff/1/src/com/google/caja/ses/repairE... src/com/google/caja/ses/repairES5.js:2819: fix('freeze'); On 2013/04/12 19:11:43, MarkM wrote: > What about fix('seal') ? Done. I knew I needed to fix every operation which sets [[Extensible]] to false, but was confused and thought that seal was not such an operation. I've also skimmed the spec to confirm that this is now a complete list, and added a note about what the list is.
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
