Firefox 44.0 doesn't execute livecode emscripten code any more

VERIFIED FIXED in Firefox 45

Status

()

Core
Canvas: 2D
VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: Hermann, Assigned: smaug)

Tracking

({dev-doc-complete, regression, site-compat})

44 Branch
mozilla47
dev-doc-complete, regression, site-compat
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44- wontfix, firefox45+ fixed, firefox46+ fixed, firefox47+ fixed)

Details

(Whiteboard: dom-triaged, gfx-noted)

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:44.0) Gecko/20100101 Firefox/44.0
Build ID: 20160123151951

Steps to reproduce:

Tried to load with the new Firefox 44.0
(and also tested with the FirefoxEdition 45.0a2 (2016-01-25))
my HTML5 emscripten generated standalones
http://hyperhh.org/html5/index-large.html


Actual results:

Loading seems to be the same as before version 44.0.
But after laoding (from server or cache) the HTML5-canvas is empty (doesn't display).

The result is the same for different OS (MacOS X, Win 10, Linux Ubuntu).
The result is the same when using hardware acceleration or not.
The result is the same when using multiprocesses support or not.


Expected results:

The versions before 44.0 Firefox were by far the best with caching and the best running browser for emscripten code generated by LiveCode's HTML5 standalone engine.
When loading from server I had < 7 seconds, from cache < 2 seconds, with Firefox only, others (Chrome, Opera, Safari) were clearly slower.
The same when running the code: Firefox before 44.0 was up to 5 times faster than the others.
Tested with this page: http://hyperhh.org/html5/SundayGameNb2-8.0.0-dp-9X.html

After loading, following error is shown in console.

> TypeError: Value can't be converted to a dictionary.
>   standalone-commercial-8.0.0-dp-9.js:1:292806

The actual code is following:
  canvas.getContext("2d",false)

The 2nd argument is not an object, and it causes the exception.


Regression range:
  https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e8c7dfe727cd970e2c3294934e2927b14143c205&tochange=b5e0aa4ec5117a4ddcb095d2fd39bcc07b9b9d4d

Backing out revision b5e0aa4ec511 [1] (bug 1215072) fixes the issue on latest m-c, but the same error is still reported to console, as it's mentioned in the bug.


HTML spec [2] says that we should propagate exception while converting the passed argument to dictionary:
> 3. Let dict be the result of converting jsval to the dictionary type
>    CanvasRenderingContext2DSettings. If this throws an exception, then
>    propagate the exception and abort these steps.

and WebIDL spec [3] says converting a boolean value to dictionary should throw an exception:
> 1. If Type(V) is not Undefined, Null or Object, then throw a TypeError.

So, this looks like a spec issue about web compat, either in HTML or WebIDL (maybe HTML?).
Will file a spec bug.


Anyway, we might be better fixing the implementation at least to align with other browsers, ahead of the spec fix, to address the regression.

[1] https://hg.mozilla.org/mozilla-central/rev/b5e0aa4ec511
[2] https://html.spec.whatwg.org/multipage/scripting.html#coerce-context-arguments-for-2d
[3] https://heycam.github.io/webidl/#es-dictionary
Blocks: 1215072
Status: UNCONFIRMED → NEW
Component: Untriaged → Canvas: 2D
Ever confirmed: true
Keywords: regression
Product: Firefox → Core
Created attachment 8714048 [details]
minimal testcase

Here's the testcase that calls |canvas.getContext("2d",false)|.

Safari, Chrome, and Edge don't throw any exception.
[Tracking Requested - why for this release]:

Regression happens from Firefox 44 (bug 1215072), that is caused by making HTMLCanvasElement.getContext to follow the spec (bug 645792 + bug 1215072), that does not match to other browsers and previous Firefox releases.
it breaks websites that rely on previous behavior.
Blocks: 645792
status-firefox44: --- → affected
status-firefox45: --- → affected
status-firefox46: --- → affected
status-firefox47: --- → affected
tracking-firefox44: --- → ?
tracking-firefox45: --- → ?
tracking-firefox46: --- → ?
tracking-firefox47: --- → ?
I would prefer if the site was fixed, given that we're just being consistent with webidl handling and this case indicates there is an error in the page's code.

But if fixing the site isn't possible, need to add some special case to the spec.
Has anyone reported to the website about the issue in its code?
(In reply to Tooru Fujisawa [:arai] from comment #4)
> it breaks websites that rely on previous behavior.
websites or a website? Do we know if this has happened more often?
(In reply to Olli Pettay [:smaug] from comment #7)
> websites or a website? Do we know if this has happened more often?

sorry, "a website" at this point.


Hermann, where does the code related to canvas come from?  is it also a part of LiveCode?
Flags: needinfo?(hh)
Whiteboard: dom-noted

Updated

3 years ago
Keywords: site-compat
Looks like Chrome does throw exception in case one does
canvas.getContext("2d", { get alpha() { throw "FooBarError"; } });
But not when just {}, or "" or 123 or false is passed as the 2nd param.
Obviously passing {} works also in Firefox since that is what the spec allow (alpha defaults to true)


Looks like chromiums code here is really odd.
https://codereview.chromium.org/795833004 made them to use the same dictionary for all the context types.
So document.body.innerHTML = "<canvas>"; document.body.firstChild.getContext("2d", { get stencil() { throw "Foobar"; } }); throws in Chrome, even though it should not per spec.
Chrome ignores non-object value, in following code.

https://code.google.com/p/chromium/codesearch#chromium/src/out/Debug/gen/blink/bindings/core/v8/V8CanvasContextCreationAttributes.cpp&q=CanvasContextCreationAttributes&sq=package:chromium&dr=CSs&l=20
> void V8CanvasContextCreationAttributes::toImpl(v8::Isolate* isolate,
> v8::Local<v8::Value> v8Value, CanvasContextCreationAttributes& impl,
> ExceptionState& exceptionState)
> {
>     if (isUndefinedOrNull(v8Value)) {
>         return;
>     }
>     if (!v8Value->IsObject()) {
>         // Do nothing.
>         return;
>     }


WebKit seems not to support 2nd argument on "2d" type.
on webgl, it converts boolean to Boolean object

https://github.com/WebKit/webkit/blob/6bc6a03d1caa8702af53d1a46114e0977ce622b9/Source/WebCore/bindings/js/JSHTMLCanvasElementCustom.cpp#L101
> static void get3DContextAttributes(ExecState& state, RefPtr<CanvasContextAttributes>& attrs)
> {
>     JSValue initializerValue = state.argument(1);
>     if (initializerValue.isUndefinedOrNull())
>         return;
>     
>    JSObject* initializerObject = initializerValue.toObject(&state);
> ...
> ...
> JSValue JSHTMLCanvasElement::getContext(ExecState& state)
> {
>     HTMLCanvasElement& canvas = wrapped();
>     const String& contextId = state.argument(0).toString(&state)->value(&state);
>     
>     RefPtr<CanvasContextAttributes> attrs;
> #if ENABLE(WEBGL)
>     if (HTMLCanvasElement::is3dType(contextId)) {
>         get3DContextAttributes(state, attrs);
>         if (state.hadException())
>             return jsUndefined();
>     }
> #endif
The official demo on LiveCode page causes same error.
  http://livecode.com/products/livecode-platform/html5/demos/calculator/
> TypeError: Value can't be converted to a dictionary.
>   standalone-community.js:1:194375

The code is also same:
> canvas.getContext("2d",false)

I guess the error in the comment #0 is the same thing.


They says that HTML5 deployment is supported from Developer Preview (DP4) that was released 2015-08-31, and it's marked as experimental.
and there seems to be no newer versions (am I correct?)

http://downloads.livecode.com/livecode/8_0_0/LiveCodeNotes-8_0_0_dp_4.pdf
> Support for the web platform using HTML5 (8.0.0-dp-4 - experimental)
> 
> The LiveCode engine will now run in web browsers that support HTML5.
> This means that you can now deploy simple LiveCode apps to users without any
> installation required.
> To deploy a stack as an HTML5 application, enable the "Build for HTML5"
> checkbox on the "HTML5" page of the standalone settings window, and then
> generate the standalone in the normal way.
> For more information on HTML5 deployment, including options for embedding
> LiveCode standalones i web pages, please see the "HTML5 Deployment" guide in
> the IDE dictionary.
> 
> Important: This feature is currently experimental. This means that it may not
> be complete or may fail in some circumstances that you would expect it to
> work. Please do not be afraid to try it out as we need feedback to develop it
> further.

so, at least for this case, it should be better contacting them and wait for the fix on their side, instead of reverting the behavior on Firefox.
(Reporter)

Comment 12

3 years ago
Hi all,

sorry for the late answer. I am an (advanced) user, not member of the LC-developer team,
and can't see at the moment what could remove the issue in the emscripten calling code.
The js exception is with ALL latest version of the LiveCode HTML5 standalone engine
(via emscripten).

This doesn't depend in any way on my special website (an alternate server with
the same content is http://hh.on-rev.com/html5/).

The Livecode-emscripten engine is open source at github
( https://github.com/livecode/livecode ).
The current LiveCode version is 8.0.0-dp13. There were bugs in dp10-dp12
(relating to speed only) that caused me to still use dp9.

Safari, Chrome and Opera work with all standalones on my site. My examples are
free of errors. One complicated example, good for testing (with respect to the canvas)
is http://hyperhh.de/html5/asianRodsHTML-8.0.0-dp-9X.html.

Microsoft products (IExplorer, Edge) didn't work at all. They load, but don't display.

Firefox before 44.0 (and even the 'derivate' Iceweasel on RaspberryPi) loaded fastest,
displayed fast. [Could't only avoid the blinking cursor at topleft of the canvas].

I informed the leader of the LiveCode developer team, he's on congress until monday
morning. But he will come in to answer your special questions.

I hope so much that this js-exception can be removed -- I'll get soon a review of the
"math-related clocks" on my site (lower half of the examples) in the american
math-online-magazin "Math Munch". I told them to tell their readers to use Firefox
because other browser are not fast enough with the emscripten output ...

Thank you very much for your interest and help.

Kind regards from Germany, Hermann
Flags: needinfo?(hh)
Thank you for the detail, and contacting them :)

The actual code seems to be in following file:

https://github.com/livecode/livecode/blob/8db7eda7f746ec7babf2e06debedf4ef746c05d3/engine/src/em-surface.js#L29
>         // Create and get a 2D rendering context for the canvas
>         var context = canvas.getContext('2d', false);
>         if (!context) {
>             context = Browser.createContext(canvas, false, true);
>         }

the code sounds like some compatibility testing thing.
anyway, if the "false" there has no meaning for other platform, just removing it will fix the issue.


Also, similar case is found in web-platform-test, and looks like have been imported to WebKit and chromium.

https://github.com/w3c/web-platform-tests/blob/master/html/semantics/embedded-content/the-canvas-element/2d.getcontext.extraargs.html#L22
https://github.com/WebKit/webkit/blob/master/LayoutTests/imported/w3c/canvas/2d.getcontext.extraargs.html#L24
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/LayoutTests/canvas/philip/tests/2d.getcontext.extraargs.html&l=15
> <p class="desc">The 2D context ignores extra getContext arguments</p>
> ...
> _assertDifferent(canvas.getContext('2d', false, {}, [], 1, "2"), null, "canvas.getContext('2d', false, {}, [], 1, \"2\")", "null");

at least the test does not match to the spec.

Comment 14

3 years ago
I'm the upstream LiveCode developer responsible for our Emscripten/JS port.  Tracking bug: http://quality.livecode.com/show_bug.cgi?id=16795
Since we're trying to figure out what to do with this 
(change the HTML spec and make Gecko to follow the updated spec, or let the web site to update its code), would help to understand why 'false' is passed as the 2nd parameter.
Peter, was that code copied from elsewhere or just some accidental mistake or what?
Flags: needinfo?(peter)

Comment 16

3 years ago
Unfortunately, I didn't write that piece of code, and the engineer who did is on a two-week vacation.  I suspect he just copied the code from an example of how to use the Canvas API.  I have no particular objection to changing it.

I'm happy to change it to (insert anything that will work in Firefox, Chrome and Safari).  What's recommended?
Flags: needinfo?(peter)
false as the second argument doesn't mean anything, so removing it would be the right thing to do.
Whether or not spec needs changes here is being discussed in https://github.com/whatwg/html/issues/595
But I think I'll patch this still in Gecko too, for now.
Whiteboard: dom-noted → dom-noted, gfx-noted
(Reporter)

Comment 19

3 years ago
Thank you all for your investigations and help.
Also I learned a lot from your discussions.
Created attachment 8715952 [details] [diff] [review]
bring back buggy behavior

I'm not happy with this, but perhaps we should do it after all.
So, let random stuff to be passed as argument, but only try to interpret it as a dictionary when it is 
an object.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ccf803dadcb9
Assignee: nobody → bugs
Attachment #8715952 - Flags: review?(bzbarsky)
Created attachment 8715953 [details] [diff] [review]
without extra newline
Attachment #8715952 - Attachment is obsolete: true
Attachment #8715952 - Flags: review?(bzbarsky)
Attachment #8715953 - Flags: review?(bzbarsky)
Whiteboard: dom-noted, gfx-noted → dom-triaged, gfx-noted
Comment on attachment 8715953 [details] [diff] [review]
without extra newline

r=me pending the spec getting sorted out.
Attachment #8715953 - Flags: review?(bzbarsky) → review+
Created attachment 8716020 [details] [diff] [review]
null value, not undefined.

Looks like bindings layer has, what I'd call a bug, an assertion where it asserts dictionary init takes null value, yet it later deals with null or undefined.
See
https://treeherder.mozilla.org/logviewer.html#?job_id=16351265&repo=try

So this just uses JS::NullHandleValue and not JS::UndefinedHandleValue
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc28eeae44f8





Approval Request Comment
[Feature/regressing bug #]: bug 1215072
[User impact if declined]: site(s) which used to work, don't anymore
[Describe test coverage new/current, TreeHerder]: The patch is adding tests
[Risks and why]: Should be low risk to bring back the old behavior (well, old behavior without the bug in it where exception wasn't handled properly)
[String/UUID change made/needed]: NA

So I consider this as a low risk enough even for mozilla-release, if we do a . release
Attachment #8716020 - Flags: review?(bzbarsky)
Attachment #8716020 - Flags: approval-mozilla-release?
Attachment #8716020 - Flags: approval-mozilla-beta?
Attachment #8716020 - Flags: approval-mozilla-aurora?
Attachment #8716020 - Flags: review?(bzbarsky) → review+
Comment on attachment 8716020 [details] [diff] [review]
null value, not undefined.

Unless the issue is severe enough (only one site is impacted so far) and the fix has been validated, I would hesitate to take this as a ride-along in 44.0.1 release. Sorry!
Attachment #8716020 - Flags: approval-mozilla-release? → approval-mozilla-release-
Comment on attachment 8716020 [details] [diff] [review]
null value, not undefined.

Fixes a recent regression, adds test coverage. OK to uplift to aurora.
Attachment #8716020 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 27

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/53a2e89326e5
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox47: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
tracking-firefox45: ? → +
tracking-firefox46: ? → +
tracking-firefox47: ? → +
Comment on attachment 8716020 [details] [diff] [review]
null value, not undefined.

Fix a regression, taking it.
Should be in 45 beta 4
Attachment #8716020 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Kohei Yoshino [:kohei] from comment #29)
> Posted the site compatibility doc:
> https://www.fxsitecompat.com/en-CA/docs/2016/htmlcanvaselement-getcontext-no-
> longer-accepts-non-object-context-attributes/

Why en-CA instead of en-US, by the way?
(In reply to Masatoshi Kimura [:emk] from comment #30)
> Why en-CA instead of en-US, by the way?

Because I live in Toronto and many subscribers are in Europe, so that en-CA/GB spellings are better for me and them :)

Comment 32

3 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/dbf841cfa0df
status-firefox46: affected → fixed

Comment 33

3 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/ca3a698f224b
status-firefox45: affected → fixed
While this is a valid issue, it does not sound so critical to warrant (inclusion in) a dot release. Wontfix for Fx44.
status-firefox44: affected → wontfix
tracking-firefox44: ? → -
(Reporter)

Comment 35

3 years ago
Thank you all again for your efforts and help, I'm impressed.

I'm 66 and have 5 children, but I'm still learning every day,
so please allow me this last question Ritu:

What are criterions for "sounding critical"?

Kind regards, Hermann
(In reply to Hermann from comment #35)
> Thank you all again for your efforts and help, I'm impressed.
> 
> I'm 66 and have 5 children, but I'm still learning every day,
> so please allow me this last question Ritu:
> 
> What are criterions for "sounding critical"?
> 
> Kind regards, Hermann

Hermann, thank you for your patience. A critical issue is something that usually impacts a significant # of end-users (think hundred thousand or millions). We used that as a data point in deciding whether to do a dot release for this issue or not.

Having said that, we now have a fix for this in Beta45. :) Could you please verify this issue is fixed as expected in a latest 45/46/47 build? Many Thanks!
Flags: needinfo?(hh)
(Reporter)

Comment 37

2 years ago
Ritu,

... accepted, of course.

I read the changed source for 45/46/47 and tried just now again "live"
FirefoxDeveloperEdition 46.0a2 (2016-03-01) from update channel aurora.
It works here fine again.

Firefox is in average 3 times faster than Safari, Chrome and Opera!!
Although the others now also improved their local chaching.

There was also a fix from LiveCode's side (essentially based on Tooru's
and Olli's hint), small but very effective team there.
So my projects run, recompiled, also with Firefox 44 again, all other's
have also to recompile or wait until visitors have Firefox 45/46/47 ...

Thank you all again. I think I got a taste of 'Why Firefox is the leading
browser'.

Hermann
Flags: needinfo?(hh)
(In reply to Hermann from comment #37)
> Ritu,
> 
> ... accepted, of course.
> 
> I read the changed source for 45/46/47 and tried just now again "live"
> FirefoxDeveloperEdition 46.0a2 (2016-03-01) from update channel aurora.
> It works here fine again.
> 
> Firefox is in average 3 times faster than Safari, Chrome and Opera!!
> Although the others now also improved their local chaching.
> 
> There was also a fix from LiveCode's side (essentially based on Tooru's
> and Olli's hint), small but very effective team there.
> So my projects run, recompiled, also with Firefox 44 again, all other's
> have also to recompile or wait until visitors have Firefox 45/46/47 ...
> 
> Thank you all again. I think I got a taste of 'Why Firefox is the leading
> browser'.
> 
> Hermann

Amazing! Thank you for a super prompt follow up.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.