[Track changes] - Reloading unrelated iframes in the host page clears the tracked changes

VERIFIED FIXED in Firefox 65

Status

defect
P1
normal
VERIFIED FIXED
9 months ago
8 months ago

People

(Reporter: cfogel, Assigned: rcaliman)

Tracking

(Blocks 2 bugs, {regression})

65 Branch
Firefox 66
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox64 unaffected, firefox65 verified, firefox66 verified)

Details

Attachments

(2 attachments)

[Affected versions]:
- Win10x64, Ubuntu 16.04 x64, macOS10.12.6

[Affected platforms]:
- Firefox: 65.0a1 (2018-12-07) 

[Steps to reproduce]:
1. Launch Firefox;
2. Access https://www.w3schools.com/tags/tryit.asp?filename=tryhtml_iframe
3. Right click on any element;
4. Click on the Inspect Element option;
5. In the inspector add any property (ex:  add to element {color: red;});
6. Click on the Changes tab;
7. Click on the page so that the inspector is no longer tagged as active;
8. Zoom in/out a couple of times;

[Expected result]:
- the tracked changes in Changes tab are still displayed;

[Actual result]:
- the tab (apparently) resets to the default state;

[Regression range]:
- bug 1506170 appears to be at fault

[Additional notes]:
- attached recording with the issue;
- Preference: devtools.inspector.changes.enabled - true | if the Changes tab not displayed in the inspector;
- managed to reproduce in other pages with iframes in them, but on a locally saved file the issue did not manifest.
Hey Razvan, mind taking a look at this as well?
Flags: needinfo?(rcaliman)
Thanks for filing, Cristi!

This is indeed an issue and it's caused by the iframes the advertising is loaded into. You can observe that in the video as well: notice the changes are lost as soon as the ad refreshes. 

The zooming causes a navigation in the ad's iframe. This triggers the "will-navigate" event on the TargetActor and trips the ChangesActor into believing the page itself navigates. This check needs to be made more strict:
https://searchfox.org/mozilla-central/source/devtools/server/actors/changes.js#32

You can replicate the issue without zooming by right-clicking the ad, in the context menu, select This Frame > Reload Frame. This also clears the Changes panel.

The zooming issue does not occur if you go to about:preferences#privacy and set Content Blocking to Strict. This blocks the ads in the mentioned page. Zooming no longer causes the ads to refresh/navigate and the issue does not occur. This explanations why the issue didn't manifest on a local file.
Flags: needinfo?(rcaliman)
Assignee: nobody → rcaliman
Priority: -- → P2
Summary: [Track changes] - zoom in pages resets the changes listed in the tab → [Track changes] - Reloading unrelated iframes in the host page clears the tracked changes
See Also: → 1513484
Status: NEW → ASSIGNED
Priority: P2 → P1
Duplicate of this bug: 1513484
Blocks: 1513940
Pushed by rcaliman@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7a5bfc35d789
Clear changes only when parent document navigates, not iframes; r=bradwerth
https://hg.mozilla.org/mozilla-central/rev/7a5bfc35d789
Status: ASSIGNED → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 66
Verified with 66.0a1 (2018-12-13) on the mentioned OSs
Status: RESOLVED → VERIFIED
Comment on attachment 9031181 [details]
Bug 1512665 - Clear changes only when parent document navigates, not iframes; r=bradwerth

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1506170

User impact if declined: Tracked CSS changes may be lost if there's an iframe on the inspected page which reloads (like ads), thus losing this data for users in that scenario.

Is this code covered by automated tests?: No

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: Yes

If yes, steps to reproduce: - Run this the address bar of a new tab:
data:text/html,<style>h1 {color: red }</style><h1>TEST</h1><iframe src="https%3A%2F%2Fwww.example.com%2F" />

- Open the DevTools Inspector
- Select the <h1> in the host document
- Switch to the the Rules panel and make any change to CSS properties
- Select the <h1> in the iframe
- Switch to the the Rules panel and make any change to CSS properties
- Right-click on the iframe on the page
- From the context menu, pick This Frame > Reload Frame
- Switch to the Changes panel

Expected: The list of changes should not be cleared. Change entries for both <h1> elements should appear.

Ideally, only the iframe changes should be cleared, but that's going to be solved in Bug 1513940. For now, we just want to ensure that changes are not lost accidentally when iframes reload.

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): The Changes clearing behaviour was already implemented prior to Firefox 65 Release. This patch only restricts its triggering as a result of the host page reload.

String changes made/needed:
Attachment #9031181 - Flags: approval-mozilla-beta?
Flags: qe-verify+
Comment on attachment 9031181 [details]
Bug 1512665 - Clear changes only when parent document navigates, not iframes; r=bradwerth

[Triage Comment]
Fixes a bug in the new track changes feature. Approved for 65.0b5.
Attachment #9031181 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I managed to reproduce the issue using Nightly (2018-12-07) and the method from comment 0 on Windows 10 x64.
Using Firefox 65.0b5 (Build ID 	20181216183345 - taskcluster) and the method from comment 0 and comment 8 on Windows 10 x64, macOS 10.13 and Ubuntu 16.04 x64, I verified the fix. The issue is not reproducing anymore.
Thank you all!
You need to log in before you can comment on or make changes to this bug.