Lots of JavaScript Error: "markupDocumentViewer is undefined" {file: "chrome://global/content/viewZoomOverlay.js" line: 63} in mochitest-bc

RESOLVED FIXED in Firefox 24

Status

()

Firefox
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Gijs, Assigned: adw)

Tracking

({regression})

unspecified
Firefox 26
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox24 fixed, firefox25 fixed, firefox26 fixed)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
Bisected on m-c, last log without this was:

https://tbpl.mozilla.org/php/getParsedLog.php?id=25125733&tree=Mozilla-Central&full=1

(push https://tbpl.mozilla.org/?rev=b3ff36cb6a1a )

first log with this was:

https://tbpl.mozilla.org/php/getParsedLog.php?id=25136627&tree=Mozilla-Central&full=1

(push https://tbpl.mozilla.org/?rev=dde4dcd6fa46 )

So it's somewhere in this merge:

https://hg.mozilla.org/mozilla-central/pushloghtml?startID=24938&endID=24939

This doesn't seem to be affecting tests much, but it's annoying when running stuff locally because you get noise in your error reporting.
(Reporter)

Comment 1

5 years ago
Shooting from the hip, I'd guess this is bug 880226. Drew, do you know more about what's going on here? (see also: bug 897410).
Flags: needinfo?(adw)

Updated

5 years ago
Blocks: 897410
Component: Untriaged → General
(Assignee)

Comment 2

5 years ago
Yes, it's bug 880226.  As Gavin says in bug 897410 comment 13, the problem is browser-fullZoom.js doing things with a browser (passing it to ZoomManager) after it's been closed or modified.  I think the right fix is to make the isCurrent getter in the token objects returned by FullZoom._getBrowserToken guard against this case by returning false, but there are still some things I don't understand about where and why it happens and the state of browsers and their properties that I'm trying to understand first.

Note to self, try push with some logging:
https://tbpl.mozilla.org/?tree=Try&rev=974ddea36efd
Assignee: nobody → adw
Blocks: 880226
Status: NEW → ASSIGNED
Flags: needinfo?(adw)
(Assignee)

Comment 3

5 years ago
Created attachment 786523 [details] [diff] [review]
patch

OK, I think what's happening is that these browsers have in fact been destructed by the time they're used in FullZoom's async CPS callbacks.  Literally, their <destructor>s have been called, and they no longer have any properties.

The "markupDocumentViewer is undefined" error confused me: markupDocumentViewer is just an XBL property getter.  It accesses browser.docShell.  So the error implies that the docshell is there, but the docshell's markup viewer isn't.  But my logging shows that browser.docShell is also undefined.  The markupDocumentViewer getter should be throwing an exception when it accesses a property on docShell, but it isn't.  Why?  Because actually the browser has been destructed (unbound?), and the XBL markupDocumentViewer getter isn't even attached to the object anymore.
Attachment #786523 - Flags: review?(mak77)
Comment on attachment 786523 [details] [diff] [review]
patch

I wonder whether browser.ownerDocument.defaultView.closed might be a better check, but I guess that's a little disconnected from what you actually care about.

We should back bug 897410's patch out of UX after landing/merging this, right?
Attachment #786523 - Flags: review?(mak77) → review+
(Assignee)

Comment 6

5 years ago
Thanks for the review.

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #5)
> I wonder whether browser.ownerDocument.defaultView.closed might be a better
> check, but I guess that's a little disconnected from what you actually care
> about.

It can't be the only check, because in these situations, the browser has no properties at all, I presume because the object has been unbound from the XBL.  So the check you suggests actually throws an error because browser.ownerDocument is undefined.  Maybe your check should be in addition to a browser.ownerDocument check, but until we start seeing more errors indicating that browsers have been closed but not yet unbound, I don't think it's necessary.

> We should back bug 897410's patch out of UX after landing/merging this,
> right?

I posted bug 897410 comment 15 about this.
(Assignee)

Comment 7

5 years ago
Landed on inbound and not fx-team only because it was more convenient for me right now.

https://hg.mozilla.org/integration/mozilla-inbound/rev/2b9d71496f60
https://hg.mozilla.org/mozilla-central/rev/2b9d71496f60
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
https://hg.mozilla.org/releases/mozilla-aurora/rev/3a5b3e09a128
https://hg.mozilla.org/releases/mozilla-beta/rev/356ac80f1a90
status-firefox24: --- → fixed
status-firefox25: --- → fixed
status-firefox26: --- → fixed
This doesn't look like something QA needs to verify. Please remove the [qa-] whiteboard tag and add the verifyme keyword if this is an incorrect.
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.