610017 - _isCmdLineEmpty should not clobber window.arguments[0]

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[:Gavin Sharp [email: [e-mail]]]

It's unexpected that this method would have side-effects, and it doesn't seem to serve a purpose, since by the time this code has run (triggered via delayedStartup), the argument will have already been handled by the window's onload handler.

Comment 1

[Michael Kohler [:mkohler]]

Attachment: Patch v1

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 645688 [details] [diff] [review]
Patch v1

Review of attachment 645688 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for tackling this, Michael.

Unfortunately we can't just remove the second argument and the local 'pinnedOnly' variable. We still need this as "aWindow.arguments[0]" affects the return value. We just shouldn't modify "aWindow.arguments" but rather save this to a local variable.

Comment 3

[Michael Kohler [:mkohler]]

Attachment: Patch v2

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 645789 [details] [diff] [review]
Patch v2

Review of attachment 645789 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +3889,5 @@
>  
>      if (!pinnedOnly) {
>        let defaultArgs = Cc["@mozilla.org/browser/clh;1"].
>                          getService(Ci.nsIBrowserHandler).defaultArgs;
> +      var windowArguments = aWindow.arguments;

Nit: please use 'let' for variable definitions. Let is the new var!

The actual problem here is that windowArguments does now contain a reference to aWindow.arguments. Modifying windowArguments[0] is therefore the same as modifying aWindow.arguments[0].

Comment 5

[Michael Kohler [:mkohler]]

Attachment: Patch v3

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 646548 [details] [diff] [review]
Patch v3

Review of attachment 646548 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Michael, great work!

Comment 7

[:Gavin Sharp [email: [e-mail]]]

This is checkin-needed, right?

Comment 8

[Ryan VanderMeulen [:RyanVM]]

(Remarkably) Green on Try.
https://tbpl.mozilla.org/?tree=Try&rev=cf7f719d7f47

https://hg.mozilla.org/integration/mozilla-inbound/rev/ebc0ba34959d

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ebc0ba34959d