988863 - Location expandos can disappear

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Bobby Holley (:bholley)]

This is a regression from bug 961204. It affects FF29 and newer.

Previously, we never preserved the Location wrapper, because it was a value prop on the Window. But in bug 961204 (FF29), we stopped doing that.

In bug 809547, we added test coverage to make sure that Location expandos don't get collected. As Peter discovered, SpecialPowers was adding edges to the Location object, pinning it alive in automation when it would actually be collected in release builds. Bug 988334 exposed this, and started making that test coverage fail.

The test coverage for this bug is effectively the test from bug 809547, which will start doing its job when we land bug 988334.

I'll upload a patch.

Comment 1

[Bobby Holley (:bholley)]

This is a web-compat regression that we should track and fix before shipping the regression (currently on beta).

Comment 2

[Bobby Holley (:bholley)]

Attachment: Preserve Location in AddProperty. v1 r=bholley

Peter wrote this patch. r=me

Comment 3

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b1b90e5fd34

Comment 4

[Bobby Holley (:bholley)]

Comment on attachment 8397845 [details] [diff] [review]
Preserve Location in AddProperty. v1 r=bholley

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 961204
User impact if declined: nasty web-compat issues. User-defined properties on a Location objects can disappear non-deterministically during GC.
Testing completed (on m-c, etc.): Just pushed to m-i.
Risk to taking this patch (and alternatives if risky): Low risk.
String or IDL/UUID changes made by this patch: None

Comment 5

[Ed Morley [:emorley]]

Backed out for:
09:29:23     INFO - #####
09:29:23     INFO - ##### Running check-expectations step.
09:29:23     INFO - #####
09:29:23     INFO - Running main action method: check_expectations
09:29:23     INFO - Reading from file /builds/slave/l64-br-haz_m-in_dep-0000000000/build/source/js/src/devtools/rootAnalysis/expect.browser.json
09:29:24     INFO - Contents:
09:29:24     INFO -  {
09:29:24     INFO -    "expect-hazards": 0
09:29:24     INFO -  }
09:29:24     INFO - Reading from file /builds/slave/l64-br-haz_m-in_dep-0000000000/build/analysis/rootingHazards.txt
09:29:24  WARNING - TEST-UNEXPECTED-FAIL 1 more hazards than expected (expected 0, saw 1)

https://tbpl.mozilla.org/php/getParsedLog.php?id=36812265&tree=Mozilla-Inbound#error0

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2d754cf4e122

Comment 6

[Boris Zbarsky [:bzbarsky]]

Presumably need to root obj?

Comment 7

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #6)
> Presumably need to root obj?

Sounds right. Running the analysis to double-check:

https://tbpl.mozilla.org/?tree=Try&rev=d60acd46b188

Comment 8

[Bobby Holley (:bholley)]

Rooting analysis is green. Relanding: https://hg.mozilla.org/integration/mozilla-inbound/rev/7cad4bd4bfe5

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7cad4bd4bfe5

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/eee14ec21647
https://hg.mozilla.org/releases/mozilla-beta/rev/b21947c608e9

Comment 11

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Is this fix fully covered by the test you've mentioned in comment 0?

Comment 12

[Bobby Holley (:bholley)]

(In reply to Florin Mezei, QA [:FlorinMezei] from comment #11)
> Is this fix fully covered by the test you've mentioned in comment 0?

Only with bug 988334 in the tree, unfortunately. This is a hard fix to QA, and I think we understand the issue well enough that we don't need to verify the bug.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Firefox 28 had this problem too.  See bug 994048.