Page MenuHomePhabricator

[Regression 1.24wmf5] OOjs UI: Firefox throws NS_ERROR_NOT_AVAILABLE when opening certain dialogs
Closed, ResolvedPublic

Description

NS_ERROR_NOT_AVAILABLE: appearing while trying to save a page in Betalabs


Version: unspecified
Severity: critical

Details

Reference
bz65373

Event Timeline

bzimport raised the priority of this task from to Unbreak Now!.Nov 22 2014, 3:14 AM
bzimport added a project: OOUI.
bzimport set Reference to bz65373.

This is not happening with Chrome though, but with Firefox

New in wmf5, happens when opening any dialog.

Correction, not ALL dialogs. The media insertion dialog isn't affected, all the others do seem to be.

Caused by the jQuery 1.11 upgrade.

Most likely, such as was with Ia7db42b7727b8ca16ae, this is one of three things:

  1. We're giving the DOM or jQuery garbage, and previous versions silently just assumed stupidity and used a default. Now it's throwing the garbage back at us. In case of Ia7db42b77, we passed a VeDmDocument object as second parameter to $() for creating a new <div>. That was a nonsensical function call. It defaulted to the global Document. In many cases this would never be merged as that would obviously conflict with iframes, but it happened to be right one here.
  • We're relying on an undocumented behaviour (e.g. an old bug, or feature was that really just tolerance or coincidence). E.g. weird code was written, it code worked, got shipped. A good example is eg. doing something like: .css( 'font-size', '100potatoes' )

or
.css( 'font-size', '100pxlz' )

Maybe it previously used parseFloat to strip off 'px' and just assumed it was px, and changed to stripping only 'px' and passing on to the DOM without taking the blame for it and we'd now be getting weird exceptions.

(example, afaik this was not an actual change).

  1. We're relying on something that was intentionally changed. Happy reading:

    http://blog.jquery.com/2013/01/15/jquery-1-9-final http://blog.jquery.com/2013/02/04/jquery-1-9-1-released/ http://blog.jquery.com/2013/05/24/jquery-1-10-0 http://blog.jquery.com/2013/05/30/jquery-1-10-1 http://blog.jquery.com/2013/07/03/jquery-1-10-2 http://blog.jquery.com/2014/01/24/jquery-1-11 http://blog.jquery.com/2014/05/01/jquery-1-11-1 http://jquery.com/upgrade-guide/1.9/
  1. Oh my, jQuery can have new bugs?

Narrowed it down...

A breakpoint doesn't seem to work as this is a lower level exception, this is most likely a bug inside Firefox as well. After this bug most of the page is unresponsive and even reloading the page in the same or a new tab doesn't work (like it has somehow frozen the network layer or no longer allows itself to make requests to this domain). So finding where in our code we go down that path is hard to find.

Inside VisualEditor, it happens for dialogs that have iframes. Specifically the kind that copies link tags. I can see that it does make requests (which get HTTP 304, as it should) for several of the parent document's stylesheets before NS_ERROR_NOT_AVAILABLE is thrown.

When using the oojs-ui dialogs demo, after having manually upgraded lib/jquery.js to jQuery 1.11.1 (without Migrate plugin), I can reproduce this exception in Firefox. But, only for the confirmation dialog. Small, medium, large, and medium footless dialogs are all fine.

Caused by

OO.ui.ConfirmationDialog.prototype.initialize

-> OO.ui.PanelLayout

(only when passing $: this.$, without it it works fine, so it is frame
related, this is OO.ui.ConfirmationDialog/OO.ui.Dialog/OO.ui.Frame.$)

-> this.$element.addClass( 'oo-ui-' + OO.ui.Element.getDir( this.$.context ) );

Removing this line makes it go away.

-> OO.ui.Element.getDir( HTMLDocument frame$context )

isDoc
obj = obj.body

-> $( HTMLBodyElement obj ).css( 'direction' )

When doing it step-by-step, the error doesn't happen.

Inside .css():

[1.8.3] jQuery.fn.css
-> curCSS

computed = window.getComputedStyle( elem, null ),
style = elem.style
if ( computed ) {

// getPropertyValue is needed for .css('filter') in IE9, see #12537
ret = computed.getPropertyValue( name ) || computed[ name ];

[1.11.1] jQuery.fn.css
-> curCSS

computed = .. || getStyles( elem );
style = elem.style

// getPropertyValue is needed for .css('filter') in IE9, see #12537
ret = computed ? computed.getPropertyValue( name ) || computed[ name ] : undefined;

-> getStyles

return elem.ownerDocument.defaultView.getComputedStyle( elem, null );

So it's no longer using the global window and pass the (foreign) node, but uses the correct window now (ownerDocument.defaultView). If anything, that should fix issues, not cause Firefox to throw.

Change 133738 had a related patch set uploaded by Krinkle:
[WIP] Element: Work around Firefox exception in getDir()

https://gerrit.wikimedia.org/r/133738

  • Bug 65401 has been marked as a duplicate of this bug. ***

Happens when opening any dialog that inherits directionality (OO.ui.ConfirmationDialog, ve.ui.MWSaveDialog, etc.)

More elaborate trace:

Error: [Exception... "Component is not available" nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)" location: "JS frame :: http://krinkle.dev/wikimedia/oojs/ui/demos/lib/jquery.js :: curCSS :: line 6128" data: no]

Not much, but new phrases that can help are "Component is not available" and "JS frame".

Possibly related:
https://bugzilla.mozilla.org/show_bug.cgi?id=358415

"And yes, I'm cool with getComputedStyle throwing something like NS_ERROR_NOT_AVAILABLE if there's nothing in the window. "

  • Bug 65407 has been marked as a duplicate of this bug. ***
  • Bug 65422 has been marked as a duplicate of this bug. ***

Change 133738 merged by jenkins-bot:
PanelLayout: Remove call to getDir()

https://gerrit.wikimedia.org/r/133738

As far as I can tell the above didn't fix the issue for opening the media edit dialog. The save dialog is now working for me (bug 65401) and mobile citation dialog (bug 65422).

Dialogs that seem to work for me:

  • Citation
  • Reference
  • Reference list
  • Media insert
  • Save
  • Inspectors all seem to work – Link, language, hieroglyphics, gallery, special character

Dialogs that do not:

  • Media edit
  • Template (transclusion)
  • Page settings

What's the plan for fixing these?

btw, since Firefox's NS_ERROR_NOT_AVAILABLE exceptions have no stack trace. This is how you catch it:

Locally modify jquery.js like this:

  • a/resources/lib/jquery/jquery.js

+++ b/resources/lib/jquery/jquery.js
@@ -6122,10 +6122,15 @@ if ( window.getComputedStyle ) {

var width, minWidth, maxWidth, ret,
        style = elem.style;

+ try {

computed = computed || getStyles( elem );
 
// getPropertyValue is only needed for .css('filter') in IE9, see #12537
ret = computed ? computed.getPropertyValue( name ) || computed[ name ] : undefined;

+ } catch (e) {
+ mw.log.warn(e);
+ throw new Error('Firefox computed styles poop');
+ }

if ( computed ) {

Then reproduce the error (preferably in debug mode), and fine a stack like the following (this one is for Media Edit dialog):

console.trace:
mw.log</log.warn() load.php:11380
curCSS() load.php:6131
.css() load.php:6723
.css/<() load.php:6872
jQuery.access() load.php:4153
.css() load.php:6873
OO.ui.Element.getDir() oojs-ui.js:304
OO.ui.GridLayout.prototype.update() oojs-ui.js:4168
OO.ui.GridLayout.prototype.layout() oojs-ui.js:4137
OoUiGridLayout() oojs-ui.js:4069
OoUiBookletLayout() oojs-ui.js:4232
ve.ui.MWMediaEditDialog.prototype.initialize() ve.ui.MWMediaEditDialog.js:150

Conclusion, like in OO.ui.PanelLayout (fixed in I31b495fc2d9725f), GridLayout, too is doing (what seems to be erroneous) style computation on a hidden frame. So the values of it are already useless, but are now throwing in Firefox.

Should probably be dealt with similarly (in case we find more).

  • Media Edit dialog mw.log</log.warn() load.php:11380 curCSS() load.php:6131 .css() load.php:6723 .css/<() load.php:6872 jQuery.access() load.php:4153 .css() load.php:6873 OO.ui.Element.getDir() oojs-ui.js:304 OO.ui.GridLayout.prototype.update() oojs-ui.js:4168 OO.ui.GridLayout.prototype.layout() oojs-ui.js:4137 OoUiGridLayout() oojs-ui.js:4069 OoUiBookletLayout() oojs-ui.js:4232 ve.ui.MWMediaEditDialog.prototype.initialize() ve.ui.MWMediaEditDialog.js:150
  • Template (transclusion) mw.log</log.warn() load.php:11380 curCSS() load.php:6131 .css() load.php:6723 .css/<() load.php:6872 jQuery.access() load.php:4153 .css() load.php:6873 OO.ui.Element.getDir() oojs-ui.js:304 OO.ui.GridLayout.prototype.update() oojs-ui.js:4168 OO.ui.GridLayout.prototype.layout() oojs-ui.js:4137 OoUiGridLayout() oojs-ui.js:4069 OoUiBookletLayout() oojs-ui.js:4232 ve.ui.MWTemplateDialog.prototype.initialize() ve.ui.MWTransclusionDialog.prototype.initialize()
  • Page settings mw.log</log.warn() load.php:11380 curCSS() load.php:6131 .css() load.php:6723 .css/<() load.php:6872 jQuery.access() load.php:4153 .css() load.php:6873 OO.ui.Element.getDir() oojs-ui.js:304 OO.ui.GridLayout.prototype.update() oojs-ui.js:4168 OO.ui.GridLayout.prototype.layout() oojs-ui.js:4137 OoUiGridLayout() oojs-ui.js:4069 OoUiBookletLayout() oojs-ui.js:4232 ve.ui.MWMetaDialog.prototype.initialize() ve.ui.MWMetaDialog.js:47

Change 134110 had a related patch set uploaded by Krinkle:
[WIP] GridLayout: Don't call getDir() during initialize

https://gerrit.wikimedia.org/r/134110

Hmm.. with https://gerrit.wikimedia.org/r/134110 applied locally, GridLayout dialogs still fail, this time a little further down the line:

demos/dialogs.html#GridDialog
curCSS() jquery.js:6131
.css() jquery.js:6723
.css/<() jquery.js:6872
jQuery.access() jquery.js:4153
.css() jquery.js:6873
OO.ui.Element.getClosestScrollableContainer() oojs-ui.js:464
OO.ui.Element.scrollIntoView() oojs-ui.js:491
OO.ui.Element.prototype.scrollElementIntoView() oojs-ui.js:603
OO.ui.OptionWidget.prototype.setSelected() oojs-ui.js:6171
OO.ui.SelectWidget.prototype.selectItem() oojs-ui.js:6556
OO.ui.BookletLayout.prototype.updateOutlineWidget() oojs-ui.js:4616
OO.ui.BookletLayout.prototype.addPages() oojs-ui.js:4511
GridDialog.prototype.initialize()

Instead of trying to fill all the wholes where we're doing style computation to early, I've revised https://gerrit.wikimedia.org/r/134110 to instead extend the time window where we can do style computation.

I don't think it was intentionally made this short. The idea was to do it at the end of initialize(), and instead, due to our large code base and sub classes, it ended up at the beginning of initialize().

This moves it back to where it belongs.

Change 134110 merged by jenkins-bot:
Window: Apply display:none after initialize, not before

https://gerrit.wikimedia.org/r/134110

(In reply to Gerrit Notification Bot from comment #25)

Change 134110 merged by jenkins-bot:
Window: Apply display:none after initialize, not before

https://gerrit.wikimedia.org/r/134110

Is this now fixed?

(In reply to Greg Grossmeier from comment #26)

(In reply to Gerrit Notification Bot from comment #25)

Change 134110 merged by jenkins-bot:
Window: Apply display:none after initialize, not before

https://gerrit.wikimedia.org/r/134110

Is this now fixed?

Yes, this should fix it.

I am still getting that error : when I try to open Media Settings Dialog, Template and Media Settings.

Also, when I try to open Language inspector.

It has not been backported to production yet (will happen soon).

The automatic deploy to beta.wmflabs.org ran 2 minute after your comment was submitted.

Yes, the dialogs are working opening for me now, but I still get this error when I click on Insert Template on Template dialog.

Also, if you just close the Template insertion dialog without adding anything, it throws the error.

Confirmed. Closing the Template dialog triggers it as well. Looks like we found another hole that needs to be plugged.

Trace:

curCSS() load.php:6129
.css() load.php:6718
.css/<() load.php:6867
jQuery.access() load.php:4153
.css() load.php:6868
OO.ui.Element.getDir() oojs-ui.js:304
OO.ui.GridLayout.prototype.update() oojs-ui.js:4170
OO.ui.GridLayout.prototype.layout() oojs-ui.js:4139
OO.ui.BookletLayout.prototype.toggleOutline() oojs-ui.js:4382
ve.ui.MWTransclusionDialog.prototype.setMode() ve.ui.MWTransclusionDialog.js:229
ve.ui.MWTransclusionDialog.prototype.teardown() ve.ui.MWTransclusionDialog.js:335
OO.ui.Window.prototype.close() oojs-ui.js:1357
OO.ui.Dialog.prototype.close/<() oojs-ui.js:1753

Change 134156 had a related patch set uploaded by Krinkle:
MWTransclusionDialog: Remove setMode() call from teardown()

https://gerrit.wikimedia.org/r/134156

Change 134156 merged by jenkins-bot:
MWTransclusionDialog: Remove setMode() call from teardown()

https://gerrit.wikimedia.org/r/134156

Change 134159 had a related patch set uploaded by Jforrester:
MWTransclusionDialog: Remove setMode() call from teardown()

https://gerrit.wikimedia.org/r/134159

All dialogs are now opening and closing without any error in Betalabs.

Change 134159 merged by jenkins-bot:
MWTransclusionDialog: Remove setMode() call from teardown()

https://gerrit.wikimedia.org/r/134159

Verified the backported fix in test2