Closed Bug 988863 Opened 10 years ago Closed 10 years ago

Location expandos can disappear


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

Not set



Tracking Status
firefox28 --- affected
firefox29 + fixed
firefox30 + fixed
firefox31 + fixed
firefox-esr24 --- unaffected
b2g18 --- unaffected


(Reporter: bholley, Assigned: bholley)



(Keywords: regression, site-compat)


(1 file)

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.
This is a web-compat regression that we should track and fix before shipping the regression (currently on beta).
Peter wrote this patch. r=me
Attachment #8397845 - Flags: review+
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
Attachment #8397845 - Flags: approval-mozilla-beta?
Attachment #8397845 - Flags: approval-mozilla-aurora?
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)

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

Sounds right. Running the analysis to double-check:
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Attachment #8397845 - Flags: approval-mozilla-beta?
Attachment #8397845 - Flags: approval-mozilla-beta+
Attachment #8397845 - Flags: approval-mozilla-aurora?
Attachment #8397845 - Flags: approval-mozilla-aurora+
Is this fix fully covered by the test you've mentioned in comment 0?
Flags: needinfo?(bobbyholley)
Flags: in-testsuite?
(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.
Flags: needinfo?(bobbyholley)
Flags: in-testsuite?
Flags: in-testsuite+
Firefox 28 had this problem too.  See bug 994048.
QA Whiteboard: [qa-]
Keywords: site-compat
OS: Mac OS X → All
Hardware: x86 → All
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.