Skip to content
This repository has been archived by the owner on Jan 17, 2023. It is now read-only.

If the Screenshots was never used, the first saved shot is wrongly displayed as you are not the full owner of it #4337

Closed
SoftVision-CosminMuntean opened this issue Apr 13, 2018 · 7 comments
Assignees
Labels
P1 [QA]:Major issue Label for QA to mark major issues logged [QA]:Verified fixed Label for QA to mark verified fixed issues

Comments

@SoftVision-CosminMuntean

[Affected versions]:

  • Nightly 61.0a1
  • Screenshots 31.3.0 dev

[Affected Platforms]:

  • All Windows
  • All Mac
  • All Linux

[Prerequisites]:

  • The "xpinstall.signatures.required" boolean pref is set to "false".
  • The "extensions.legacy.enabled" boolean pref is set tot "true".
  • Have the Screenshots 31.3.0 dev version installed from here, on a new clean profile.
  • Screenshots was never used.

[Steps to reproduce]:

  1. Open the Firefox browser with the profile from prerequisites and navigate to any website.
  2. Click the "Take a Screenshots" button from "Page Actions" menu.
  3. Skip the Screenshots Onboarding tour.
  4. Perform and save a selection on the page.
  5. Observe the saved screenshot page.
  6. Click the "My Shots" button from the top left corner of the page and observe the behavior.

[Expected result]:
Step 5. The saved shot page is correctly displayed as you are the owner of the shot.
Step 6. You are redirected to the "My Shots" Page.

[Actual results]:
Step 5: Wrong icon is displayed on the "My Shots" button and the "Flag" button is wrongly displayed instead of the "Delete" button.
Step 6: The "My Shots" button wrongly redirects you to the Onboarding page instead of the "My Shots" page.

[Additional Notes]:

  • The issue is no longer reproducible if the page is refreshed.
  • The issue is only reproducible for the first saved shot.
  • The issue is not reproducible using Screenshots 31.1 production version from latest Nightly.
  • Here are the errors displayed in browser console:
    owner
  • Here is a screen recording with the issue:
    first saved shot issue
@ghost ghost modified the milestones: Sprint 10 (61-2) ⛅, Sprint 11 (61-3) 👗 Apr 16, 2018
@ghost ghost added the P1 label Apr 16, 2018
@chenba chenba self-assigned this Apr 16, 2018
@chenba
Copy link
Collaborator

chenba commented Apr 17, 2018

Note that this bug does not occur consistently. If wantsauth has not received the auth info from sitehelper by the time the shot controller checks for the auth info, the more correct output is rendered when the shot controller's render() is called a second time. The re-rendering is slow enough to be visible. The rendered content is still not 100% correct though: the edit button does not have the first time * highlight. The model data hasn't change in that second render() call.

The screen recording above shows flag button rendered with the edit button; that should not be possible at all...

Downgrading react and react-dom back down to the previous 15.6.1--they were upgraded as part of #4280--"fixes" this bug. The correct buttons and My Shots link are rendered. If render() is called a second time due to the race condition described above, then * highlight for the edit button is displayed. The message Warning: React attempted to reuse markup in a container but the checksum was invalid. This generally means that you are using server rendering and the markup generated on the server was not what the client was expecting... is also displayed in the console if the second render() is called.

The server side rendering appears to be consistent and correct. Because the user cookie hasn't been set by the time of the first request to the first shot, isOwner is false and the flag button is rendered.

cc @ianb

@chenba
Copy link
Collaborator

chenba commented Apr 18, 2018

As @Softvision-CristinaBadescu found in #4344, bug 1452496 is the source of the user cookie issue.

tl;dr Two things changed:

  • the user cookie has not been set by the time the user visit the first shot for the first time because of bug 1452496, causing isOwner to be false on the server.
  • the react upgrade is causing the client side rendering to mix content where isOwner is true (the edit button) and false (the flag button and the link to the homepage instead of shots index) together.

There is a third factor: the race condition that affects the second point above. But that's not new.

@chenba
Copy link
Collaborator

chenba commented Apr 18, 2018

Filed PR #4345 to fix the issue with the incorrect link and button being rendered. Once we have a workaround for the cookie issue, then we'll try upgrading again.

@jaredhirsch
Copy link
Member

@chenba The upstream bug is fixed and the fix has been uplifted to 60. Do we still need #4345?

@chenba
Copy link
Collaborator

chenba commented Apr 18, 2018

I still see the problem in the latest Nightly. I don't think Beta allow unsigned addons, so I can't verify there.

@chenba
Copy link
Collaborator

chenba commented Apr 20, 2018

This bug is no longer reproducible in the latest Nightly.

@chenba chenba closed this as completed Apr 20, 2018
ianb added a commit that referenced this issue Apr 23, 2018
chenba added a commit that referenced this issue Apr 23, 2018
Revert "Downgrade react to latest 15.x.x. (#4337)"
@SoftVision-CosminMuntean
Copy link
Author

I have retested this issue using the latest Nightly (61.0a1 Build ID: 2018-04-24) with latest Screenshots dev version (32.2.0) installed and also on the provided Nightly try build with Screenshots 32.1.0 version and the issue is no longer reproducible.

Tested on Mac 10.12, Windows 7 64 and Arch Linux.

@SoftVision-CosminMuntean SoftVision-CosminMuntean added the [QA]:Verified fixed Label for QA to mark verified fixed issues label Apr 26, 2018
@SoftVision-CosminMuntean SoftVision-CosminMuntean added the [QA]:Major issue Label for QA to mark major issues logged label May 7, 2018
testeaxeax pushed a commit to testeaxeax/screenshots that referenced this issue Jun 7, 2018
testeaxeax pushed a commit to testeaxeax/screenshots that referenced this issue Jun 7, 2018
testeaxeax pushed a commit to testeaxeax/screenshots that referenced this issue Jun 7, 2018
testeaxeax pushed a commit to testeaxeax/screenshots that referenced this issue Jun 7, 2018
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
P1 [QA]:Major issue Label for QA to mark major issues logged [QA]:Verified fixed Label for QA to mark verified fixed issues
Projects
None yet
Development

No branches or pull requests

3 participants